-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Core: Vite integration #495
base: main
Are you sure you want to change the base?
Conversation
@50rayn Another great contribution, thanks! The build looks good, but this dependencies should not be bundled:
To be honest, I don't remember. You can try removing it and see what happens.
I agree with you. At the beginning of the project it was convenient this way but now it's not. We can change it to a single |
Regarding both questions, I can do separate PRs. There, I'll check all the necessary changes. About those dependencies, it is fixed by ignoring the |
c247790
to
deaaffd
Compare
@50rayn Thanks! We're planning to release Unovis 1.5 soon. Will get back to this right after |
4044248
to
385d8ae
Compare
Hi @50rayn, Thanks for the contribution! There seems to be an issue with dynamic import moving from rollup to vite. I released a beta version |
Confirmed. There's a problem with imports that end with the import { Color as r } from "/Users/nikitarokotyan/unovis/node_modules/three/src/math/Color.js"; Update: Even if you remove the |
@50rayn I've tried to reconfigure the common js plugin a couple of times but without success. So now I'm out of ideas. |
385d8ae
to
9d0bc2f
Compare
Hello everyone.
|
Thanks for the update. I will test it this week. |
@50rayn have you tried to do And if you look at the pipeline: https://github.com/f5/unovis/actions/runs/13773780019/job/38734324725?pr=495 |
@@ -64,7 +64,6 @@ module.exports = { | |||
'@src': path.resolve(__dirname, './src/'), | |||
|
|||
// Unovis Core | |||
'@unovis/ts': path.resolve(__dirname, '../ts/src/'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should do this — we want the dev app to import Unovis from the source otherwise we'll have to continuously build the package during development
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I didn't know that this was an issue. I looked at some monorepos like histoire or vue-devtools. If they are going to make some changes in core, let's say ts, they turn on watch mode for both packages. if no, they just build the ts, and run in watch the dev. This way, it can also be tested on the types and how everything is imported/exported. Also, each package should act more independently. Afterwards, I see no sense in monorepo structure. BTW, such an approach can block the integration of PNPM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see quite a lot of value in having HMR while developing new components. Why do you think it may block PNPM integration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prior Webpack alias pointed to the src/
folder of @unovis/ts
which was beneficial for HMR. That said, this can lead to problems with PNPM because of its symlink and dependency management. Resolving @unovis/ts
through package.json
guarantees symlink support within PNPM and better dependency resolution. In next commits I completely separated the packages from eachother from linking directly and let them be linked through packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I'm a bit concerned about these massive changes, and before we merge them, we'll need to thoroughly test how they would affect the dev workflow. Most likely I won't have any bandwidth for this within the next few months, maybe @lee00678 will.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing. It would be perfect if somebody else tested it, just in case I missed something.
The next steps will be:
- PNPM support
- Create a common vite.config (mergeConfig)
- Integrate unit-testing (vitest monorepo)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try to review this in the next 2 weeks.
af357ac
to
ffd4f67
Compare
In this PR done a couple of things:
During integration, I met some misunderstandings, like:
Why in
.npmrc
do we havelegacy-peer-deps=true
?Because of this flag, I could not install the latest version of
vite-plugin-dts
.In
@unovis/ts
in tsconfig, do we have a lot ofpaths
configured? Why?It makes more ambiguous the imports in the files. You may not understand it's imported from a file, module, and alias. I'd add just one,
"@/*": ["src/*"]
. It's more common for most users.I wanted to be sure that I could open some PRs for each of the mentions, but first of all, I need to be sure if it makes sense.