-
-
Notifications
You must be signed in to change notification settings - Fork 6k
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
refactor: remove build time pre-bundling #15184
refactor: remove build time pre-bundling #15184
Conversation
|
I don't know why the docs are failing in netlify, they are building correctly locally. |
test.runIf(!isBuild)('message from require-external-cjs', async () => { | ||
await page.goto(url) | ||
expect(await page.textContent('.require-external-cjs')).toMatch('foo') | ||
}) |
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.
@sapphi-red I had to skip this test during build. It seems it only works with deps optimization during build.
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's fine for now. To make this work, we'll need to convert import foo from 'foo'
into import module from 'node:module'; const foo = module.createRequire(import.meta.url)('foo')
in this case.
this could break vite-plugin-svelte, which checks optimizeDeps.disabled but doesn't know about optimizeDeps.noDiscovery. If i understand it correctly noDiscovery isn't the same as disabled. noDiscovery prevents the auto-scanner, but user config/plugins like v-p-s would still be able to add to optimizeDeps.include. whereas disabled=true would turn it off completely. The documentation here https://vitejs.dev/config/dep-optimization-options.html#optimizedeps-disabled only mentions that optimizing in build mode is experimental, not that optimizeDeps.disabled as a flag itself is. So to not break, i suggest to keep the disabled option, but change it so that false/'dev' log a warning that they are no longer supported. and with 6.0 we can change it to |
/ecosystem-ci run vite-plugin-svelte |
📝 Ran ecosystem CI on
|
Edit: fixed in tsx, install [email protected] For docs, you can set NODE_VERSION to 20 in netlify.toml. Node v18.19 changed some stuff about loaders that's breaking tsx. Created an issue upstream - privatenumber/tsx#421 |
/ecosystem-ci run |
5da3702 brings back
Just for reference, both the docs and the types have |
📝 Ran ecosystem CI on
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
/ecosystem-ci run vite-setup-catalogue |
📝 Ran ecosystem CI on
|
@@ -141,7 +141,7 @@ test('vue + vuex', async () => { | |||
|
|||
// When we use the Rollup CommonJS plugin instead of esbuild prebundling, | |||
// the esbuild plugins won't apply to dependencies | |||
test('esbuild-plugin', async () => { | |||
test.runIf(isServe)('esbuild-plugin', async () => { |
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 really appreciate your work, but I have some feedback I'd like to share. There are some custom esbuild plugins in my app to handle special files in dependencies, as described in docs. If the pre-build feature is removed at build time, is there any other way to keep the custom esbuild plugin consistent in build mode?
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.
You'll need to have the same custom handling implemented as a rollup plugin. It would be good if you link to the way you are using it for reference, but I think it would be hard to justify the complexity this PR is removing as it can be still make to work. And later on if we move to Rolldown instead of esbuild, then you will get the consistency back for dependency handling.
Even without If we keep |
indifferent whether if should be called disabled or enabled. Why would a warning for disabled: false be a problem though? you'd only warn in build mode. and someone who uses disabled: false can remove it to get rid of the warning |
We have these values to take into account:
If we keep disabled, we can't properly warn about the commonjs plugin being disabled either. As there may be valid reasons for that. Using |
I think I'm leaning towards keeping But anyways their build setup may likely fail anyways so we couldn't emit a warning + do something to fix it. Re |
I think if we keep If we keep it, we could have a transition period, in 5.1 we emit the warning for Given the low usage of build time optimization, maybe it is better to let projects fail and don't warn if we want to keep the
Ya, me too. But I think it is ok if the resolved config properly sets the value and |
I don't quite understand this part. We should allow Personally I'm also fine with no warning at all too. The build could fail, and they could check the changelog for what caused it to fail. I guess the only concern is that if it's hard to figure out what failed after they upgraded all deps, but they know the risk of using experimental features 😄 |
We can detect this, but if we want |
Yeah I'm also fine doing that and then removing it in a minor. I was thinking the chances of false positive would be low if we did the warning. But stabilizing it without the gotchas is a good point. We could both stabilize and remove the warning together in a future minor to justify it. |
@dominikg @bluwy in the previous team meeting, we discussed this PR and decided that if we need a way to disable the optimizer, we better go with a clean new flag Given that |
as mentioned earlier disabled/enabled and noDiscovery are different features really. That being said maybe a full disable is a rare usecase and trimming the option helps keeping the option count manageable? I'm still slightly in favor of keeping it, but wont object removing |
Ok, I think we should deprecate it for now and we can gather more feedback from people as they will see the notice and hopefully will reach out to us about it. |
If it's decided to deprecate Before merging though, it would be nice to address #15184 (comment), and potentially the other conversations above if they're still relevant. |
/ecosystem-ci run |
📝 Ran ecosystem CI on
|
The current PR has removed the pre-bundling behavior from the build phase, so why do we still need to retain the following logic in the build phase? vite/packages/vite/src/node/build.ts Lines 527 to 529 in 3fceb14
This will make the build phase execute pre-bundling actions by default, what does this mean? I'm not sure if there is any other reason. |
Closes #8464
Closes #9171
Closes #9549
Closes #13018
Closes #14106
Description
Starting from #8280, we added experimental support for build time deps optimization. An idea by Evan, that was originally proposed at #4921. The idea was sound, removing a big difference between dev and build (no
@rollup/plugin-commonjs
, same esbuild pipeline for dependencies in both cases) and hopefully speeding up processing on warm builds (once deps were cached).In practice, the extra complexity of doing deps optimization at build time and added points of failures made this feature stay experimental until now, seeing little usage. We had issues with rollup not being able to correctly tree-shake dependendencies (that we probably could have solved eventually, but is a good example).
With Rollup 4 switching its parser to native, and Rolldown being worked on, both the performance and the dev-vs-build inconsistency story for this feature are no longer valid. If we want to focus on improving dev/build consistency, then using Rolldown for prebundling during dev and Rolldown as-is during build is a better bet moving forward. Rolldown may also implement caching in a way that is a lot more efficient during build than deps prebundling.
This PR removes the feature, and also removes
optimizeDeps.disabled: 'build' | 'dev' | boolean
with it. We now haveoptimizeDeps.noDiscovery
. So the optimizer can be disabled by settingoptimizeDeps.noDiscovery: true
and leavingoptimizeDeps.include
empty.The PR keeps SSR deps optimization during dev. In this case,
ssr.optimizeDeps.noDiscovery
is alwaystrue
, and the user must manually add deps to optimize tossr.optimizeDeps.include
. When we added this feature, we madenoExternal
auto-populatessr.optimizeDeps.include
. This PR removes this heuristic, because it means we will need a new config option to disable the SSR dev optimizer and also forcedexclude
to work differently. Given thatnoExternal
/external
may also be reviewed later, it is better to keepinclude
explicit. The user can easily share the array if they would like allnoExternal
deps to be optimized. @bluwy raised the question if it is justified to keep deps optimization during SSR dev. The complexity is fairly low and it is useful to workaround some issues, so for now, it is kept in this PR. It is still experimental, so we can review and remove it later on too if that is desired.Related feedback discussions:
What is the purpose of this pull request?