Skip to content
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

Merged
merged 1 commit into from
Sep 6, 2021
Merged

fix(plugin-legacy): skip in SSR build #4536

merged 1 commit into from
Sep 6, 2021

Conversation

andylizi
Copy link
Contributor

@andylizi andylizi commented Aug 8, 2021

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?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@Shinigami92
Copy link
Member

/cc @brillout

@tjk
Copy link
Contributor

tjk commented Sep 2, 2021

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 []) -- would mean these checks wouldn't have to be spread out.

On a slightly tangential note, the config.lib.build check and error in legacy-post-process doesn't happen until enforce: "post" but that could be known upfront so we don't have to delay the failure. Is that correct?

Also there is currently a apply option on Plugin... could that be extended a bit so more simple config setting checks can be done by vite instead of plugins themselves?


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',
Copy link
Member

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
}

🤔

Copy link
Contributor

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.

Copy link
Contributor

@brillout brillout left a 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 😍.

@andylizi
Copy link
Contributor Author

andylizi commented Sep 6, 2021

I've submitted #4857 for apply function, and we can rewrite this PR pending that.

@brillout
Copy link
Contributor

brillout commented Sep 6, 2021

Neat 👌.

we can rewrite this PR pending that.

We can already rebase it against your other PR now, even though it's not merged yet.

@patak-dev
Copy link
Member

Thats great @andylizi 👍

We can already rebase it against your other PR now, even though it's not merged yet.

Please dont. Lets merge this one so we can fix #4818, and release tomorrow. Getting approval for apply will take some days

@patak-dev patak-dev merged commit 1f068fc into vitejs:main Sep 6, 2021
@andylizi andylizi deleted the legacy-skip-ssr branch September 6, 2021 10:59
@andylizi
Copy link
Contributor Author

andylizi commented Sep 6, 2021

Uh, I just realized, there may be a bit of a problem with the current implementation.……

const legacyEnvPlugin = {
name: 'legacy-env',
config(_, env) {
if (env) {
return {
define: {
'import.meta.env.LEGACY':
env.command === 'serve' ? false : legacyEnvVarMarker
}
}
} else {

I didn't gate this part with a SSR check, so when building in SSR mode, it will inject the marker as import.meta.env.LEGACY, but cannot later replace it due to relevant parts being disabled.

if (raw.includes(legacyEnvVarMarker)) {
const re = new RegExp(legacyEnvVarMarker, 'g')
let match
while ((match = re.exec(raw))) {
ms.overwrite(
match.index,
match.index + legacyEnvVarMarker.length,
`false`
)
}
}

So if I'm understanding the process correctly (haven't tested), this will cause SSR build to throw ReferenceError: __VITE_IS_LEGACY__ is not defined…… And it wasn't caught by tests because we didn't test legacy with SSR.

Oops? 😅

@patak-dev
Copy link
Member

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 playground/plugin-legacy just to avoid these regressions in the future.

@andylizi
Copy link
Contributor Author

andylizi commented Sep 6, 2021

Sure. #4861

aleclarson pushed a commit to aleclarson/vite that referenced this pull request Nov 8, 2021
aleclarson pushed a commit to aleclarson/vite that referenced this pull request Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants