-
Notifications
You must be signed in to change notification settings - Fork 137
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
fix vite watch mode dep optimizer #1876
Conversation
495565b
to
e5c7c9e
Compare
This is how things used to work. Doing it correctly requires more than just links to the rewritten packages, in general it requires you to rewrite the entire node_modules graph in order to make every package see all the correct dependencies. And I don't think it's necessary. I think that from the perspective of the rewritten-app, the rewritten-packages don't appear to be inside node_modules. But when we stop rewriting the app, they will. |
The problem is that vite needs a bare import and the importer cannot be in node modules. What we are currently doing for resolving rewritten-packages is doing a rehome inside the rewritten-packages, onto moved-target.js https://github.com/embroider-build/embroider/blob/main/packages/core/src/module-resolver.ts#L954 And here is how vite checks if it should optimise:
When we are doing a rehome, the importer is in node_modules... So rhat doesn't work https://github.com/vitejs/vite/blob/6c323d5b3ab3cdf81d21bbe965ed3c36aa7f0589/packages/vite/src/node/plugins/resolve.ts#L868 So it's not a problem of rewritten-app. |
6550053
to
41ac14e
Compare
packages/core/src/module-resolver.ts
Outdated
return resolution; | ||
case 'ignored': | ||
case 'not_found': |
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.
ignored should fallback resolve
@@ -25,7 +25,11 @@ export default class Package { | |||
|
|||
@Memoize() | |||
protected get internalPackageJSON() { | |||
return JSON.parse(readFileSync(join(this.root, 'package.json'), 'utf8')); | |||
try { | |||
return JSON.parse(readFileSync(join(this.root, 'package.json'), 'utf8')); |
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.
workaround for fromFile pointing at /app/tests/package.json and vite/deps
c6e0aa3
to
a6a07a4
Compare
7768bde
to
cde544d
Compare
84415eb
to
d937128
Compare
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.
This PR should be split up into individual PRs that address single issues that are verifiable with tests. Especially now that #2029 is merged we should be methodical with how we proceed
1854736
to
e7bd441
Compare
0722d6a
to
a3e8c8d
Compare
7df03c5
to
0dbb19d
Compare
9c9c6c6
to
101a4b1
Compare
af4648d
to
3e13a8d
Compare
3e13a8d
to
5d2b42b
Compare
fixed in #2135 |
dep optimizer has multiple phases
This is the order
Currently this is broken because of 2 things. app is in rewritten-app. It's actually easy to fix by changing to importer to the original app. Only needed when resolving bare imports.
The more difficult one is rewritten-packages. They must be node resolvable from original app. They are not currently.
I suggest to create a link from
@embroider/rewritten-packages
to the one inside . embroider and rewrite specifiers accordingly.It currently looks like it's working, because the initial scan detects the deps. But any new dep added later will not be optimized.
fixes: