-
-
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
fix(plugin-legacy): skip in SSR build #4536
Conversation
/cc @brillout |
A resolved config doesn't change throughout the build, right? It's a tad unfortunate that in the "framework" style of plugin insertion, there is no support for resolving the config once and then nesting an array of plugins underneath that (or in this case just returning On a slightly tangential note, the Also there is currently a That aside, I haven't done a proper check of whether all these checks are correct but I'm very much in support of disabling plugin-legacy if it's not needed at all (and causing errors, see #4818)! |
@@ -97,7 +97,7 @@ function viteLegacyPlugin(options = {}) { | |||
apply: '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 guess ideally we could have some feature extends apply, like
apply: 'client-build' | 'ssr-build'
but in that case, maybe a function would be more flexible:
apply(config) {
return config.command === 'build' && !config.build.ssr
}
🤔
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'm voting for a function; I can see other scenarios where an apply function would be helpful.
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 agree that we should disbale the plugin for SSR but that we should accomodate a nicer way to disable plugins.
The current implementation is brittle. An apply()
function would be a rock-solid solution 😍.
I've submitted #4857 for |
Neat 👌.
We can already rebase it against your other PR now, even though it's not merged yet. |
Uh, I just realized, there may be a bit of a problem with the current implementation.…… vite/packages/plugin-legacy/index.js Lines 451 to 462 in 1f068fc
I didn't gate this part with a SSR check, so when building in SSR mode, it will inject the marker as vite/packages/plugin-legacy/index.js Lines 252 to 262 in 1f068fc
So if I'm understanding the process correctly (haven't tested), this will cause SSR build to throw Oops? 😅 |
Nice catch @andylizi! Would you like to add these tests to the playgrounds? Since plugin-legacy takes some time to spin, it would be interesting to see if we could add a basic SSR setup to the current |
Sure. #4861 |
Description
Disable
plugin-legacy
in SSR build, since it's obviously not needed, and the output won't be used in html files anyway.What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).