-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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: optimizeDeps.include missing in known imports fallback #7218
Conversation
@DylanPiercey this PR doesn't fix marko when running a fresh SSR build in a project where there was never a dev run (so the cache dir with pre-bundling metadata is not present) because in the marko vite plugin you are only populating @bluwy I marked as ready for review, I think we should merge this one as explained in my previous comment. |
packages/vite/src/node/build.ts
Outdated
const deps = (await scanImports(config)).deps | ||
await addManuallyIncludedOptimizeDeps(deps, config) | ||
knownImports = Object.keys(deps) |
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 think it would be nice if scanImports
handle the manually included optimize deps too so we can prevent mutating deps
, and have one function to do it all. We would need to refactor a bit of how we report missing deps in the optimizer, but I think it looks a bit cleaner. Curious on your thoughts or caveats you see.
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 thought about this, and it was the first thing I tried. But I don't know if we should start throwing on missing imports during build, so we should keep that logic in the optimizer instead of the scan phase. What we could do is to create a new function in the optimizer:
export function findKnownImports(config: ResolvedConfig) {
const deps = (await scanImports(config)).deps
await addManuallyIncludedOptimizeDeps(deps, config)
return Object.keys(deps)
}
So we simplify the code on build.ts, and we avoid exposing the internal addManuallyIncludedOptimizeDeps
function. What do you think?
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 think that's fine, but I was hoping we could refactor this part together as well. E.g. scanImports
would return { deps, missingIds, missingOptimizeDeps }
and we log it out later. So we keep scanImports
as the sole function for scanning. I'm happy to merge this too and explore this refactor later though if you prefer.
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.
Im afraid of throwing a new exception during build. Lets merge this one and maybe you can later do a refactor PR so we check how it would work. We could be more bold for 3.0 also
Description
While debugging the issue fixed in #7214, I found that the same error can be reproduced in the current Marko test suite against vite 2.8.x (or any prev version) if you comment the dev step and we only test the build step.
If you don't run vite dev, the pre-bundling step isn't run, so the _metadata.json with the known imports isn't available and the build process then runs a fresh scan phase. For some reason, this scan phase seems to be given a different set of deps because I ended up with the same error then we are seeing in vite-ecosystem-ci. We need to investigate that afterward.
The issue was that
optimizeDeps.include
was being ignored in the build known imports fallback. Tested this PR against the marko CI removing the dev step and it is working correctly.What is the purpose of this pull request?