Capture new optimized deps #2135
Conversation
|
|
||
| debug('maybeCaptureNewOptimizedDep: doing re-resolve for %s ', foundFile); | ||
|
|
||
| let jumpRoot = join(tmpdir, 'embroider-vite-jump', hashed(pkg.root)); |
There was a problem hiding this comment.
Ah, nice trick
There was an advantage in the previous implementation that it was doing it only for rewritten packages, so auto-upgraded.
It might be that now this will also optimise other deps?
If a dep is excluded in vite, the default behaviour is to also exclude the dependencies of it. Because its in node_modules.
If this really does optimise deps of excluded deps i think it would be nice and definitely a candidate for another vite plugin :)
There was a problem hiding this comment.
There was an advantage in the previous implementation that it was doing it only for rewritten packages, so auto-upgraded.
Yeah, I think that probably left a bug for native v2 addons, since they also need this behavior but won't get rewritten. Consider your app depends on addon-one which depends on addon-two and both are native v2 addons so neither is rewritten.
During dev if you add a new usage of a component from addon-two, module resolving will look like:
- defaultResolve runs for
your-app/components/the-new-oneand doesn't find it. - fallbackResolve finds it in the mergeMap and retries with
request.alias('addon-two/_app_/components/the-new-one.js').rehome('/path/to/your-app/node_modules/addon-one/package.json'). The rehome here is chosen to be in a package that can definitely resolveaddon-two. - defaultResolve finds the file, and the specifier is a bare import, but the fromFile includes
node_modules, so you get an un-optimized dependency.
If a dep is excluded in vite, the default behaviour is to also exclude the dependencies of it. Because it's in node_modules.
That's a good point, and another reason why vite's isInNodeModules check is silly. It should really be crawling the package graph to decide where the boundary between not-optimized and optimized packages is.
Yeah, a plugin could fix that problem for everyone, although IMO it would be better to fix it inside vite. I think their package info caches are equivalently powerful to the packageCache in embroider, so could solve the problem without a lot of extra infrastructure.
|
@patricklx I don't understand yet why the new tests are timing out on windows. It looks like we're failing to shutdown the server between tests, since the next run logs that it had to try multiple ports before finding a free one. |
Yeah, I also saw that. It has been working before and i didn't change the tests. Maybe vite changed something? |
|
Running locally on windows I think the problem is that there's a module cycle in the tests, and that cycle apparently evaluates in a different order on windows. The cycle is that the service in I think if we change the test to use a different example file in app rather than app.js it will address the cycle. |
|
The cycle problem was real and I could reliably reproduce it and it's fixed. I've seen the windows tests passing locally. I'm also seeing inconsistent behaviors. It looks like sometimes the prebuild hasn't really finished successfully, because you get failures during dep optimization that can only be explained by missing prebuild output. On CI it's so slow I've never seen it finish. So I'm skipping them on windows. |
Yeah, i saw it pass sometimes, took like 16min... |
|
@patricklx thanks for your work on this |
Builds on the tests and findings in #1876, with a retry-based implementation.
The strategy here is that a newly added dep is a pretty rare occurrence, so it's better to let most resolutions run normally and only check them after the fact to see if they could have been new un-optimized dependencies, then do a second resolve for them that's adjusted so to conform to vite's rules about what is allowed to be optimized, as explained by @patricklx in #1876.
We really only need to hide any
node_modulesin thefromFile. The rest (like always having a non-relative specifier) is already handled in core module-resolver.