embroider-build / embroider

Compiling Ember apps into spec-compliant, modern Javascript.
MIT License
329 stars 137 forks source link

fix vite dep optimizer #1876

Open patricklx opened 1 month ago

patricklx commented 1 month ago

tests: https://github.com/embroider-build/embroider/pull/1876/files#diff-3e325354530b11c4e6b3ab941d93a8fe2ceb26087f9c13856c9d8684364a3fb3R62

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:

ef4 commented 1 month ago

I suggest to create a link from @embroider/rewritten-packages to the one inside

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.

patricklx commented 1 month ago

I suggest to create a link from @embroider/rewritten-packages to the one inside

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 I'm aiming to do here is to create a resolved bare specifier. So instead of having ./node_modules/.embroider/rewritten-packages/pgk1/node_mod/pkg/my-file.js We have @embroider/rewritten-packages/pgk1/node_mod/pkg/my-file.js And things will work

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 and https://github.com/vitejs/vite/blob/6c323d5b3ab3cdf81d21bbe965ed3c36aa7f0589/packages/vite/src/node/plugins/resolve.ts#L353

So it's not a problem of rewritten-app.