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

feat: add experimental option to skip SSR transform #11411

Merged
merged 1 commit into from
Jan 18, 2023

Conversation

cyco130
Copy link
Contributor

@cyco130 cyco130 commented Dec 18, 2022

Description

This PR adds an experimental option to skip SSR transform so that we can use Vite directly in Node's (currently experimental) ESM loaders. You can think of it as a minimal alternative to #9740. This tiny change enables the ecosystem to experiment with this approach without modifying Vite's core.

Building a Node ESM loader on top of Vite using this approach would render #3288 and #3928 less urgent.

The PR also exposes shouldExternalizeForSSR which does most of the work in deciding whether a dep should be externalized. Exposing it makes it possible to use the same logic in loaders. (not necessary, import analysis plugin takes care of it, a simple "is first char dot or slash" check is enough)

Additional context

In SSR dev mode, Vite transforms ESM imports and exports into special forms like __vite_ssr_import__ and __vite_ssr_exports__. This PR adds an option to skip that transform so that transform output is valid native ESM. Of course it breaks normal operation but it allows using Vite in Node's ESM loaders.


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 PR Title 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.

@cyco130
Copy link
Contributor Author

cyco130 commented Dec 18, 2022

A good loader implementation also requires access to the shouldExternalizeForSSR function so that it can let Node handle the loading when appropriate. The alternative would be to duplicate the exact logic. Ideas on how to expose it are welcome. (On my local branch, I just exported it with an @experimental annotation).

bluwy
bluwy previously approved these changes Dec 19, 2022
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a nice middle ground for now

@bluwy bluwy added the p2-nice-to-have Not breaking anything but nice to have (priority) label Dec 19, 2022
@bluwy
Copy link
Member

bluwy commented Dec 19, 2022

A good loader implementation also requires access to the shouldExternalizeForSSR function so that it can let Node handle the loading when appropriate. The alternative would be to duplicate the exact logic. Ideas on how to expose it are welcome. (On my local branch, I just exported it with an @experimental annotation).

When does something need to be externalized or not? IIUC this feature requires externalizing everything by default, which today works partially for non-linked packages.

@cyco130
Copy link
Contributor Author

cyco130 commented Dec 19, 2022

When does something need to be externalized or not?

I think most things are externalized for SSR by default. But you can opt out of externalization with ssr.noExternal, ssr.optimizeDeps or you can force externalization with ssr.external. Rakkas, for example, uses ssr.noExternal to force Vite to process Rakkas's own library so that it can access virtual files etc.

shouldExternalizeForSSR does most of the work in deciding whether a dep should be externalized. Exposing it would make it possible to use the same logic in loaders.

(I just pushed a version that exposes it, updating the OP).

@patak-dev patak-dev added p3-significant High priority enhancement (priority) and removed p2-nice-to-have Not breaking anything but nice to have (priority) labels Dec 19, 2022
@cyco130
Copy link
Contributor Author

cyco130 commented Dec 19, 2022

I published this under @cyco130/vite and created a proof-of-concept repo here: https://github.com/cyco130/vite-loader-poc

It doesn't cover many edge cases but it works. Breakpoints and sourcemaps work too.

(Doesn't work on StackBlitz or CodeSandbox, not sure why yet)

Update: A more complete example with the upcoming vavite loader (cyco130/vavite#17), vite-plugin-ssr, and React: https://github.com/cyco130/vavite-vps-loader

Update 2: Another one, this time with SvelteKit.

@bluwy
Copy link
Member

bluwy commented Dec 20, 2022

shouldExternalizeForSSR does most of the work in deciding whether a dep should be externalized. Exposing it would make it possible to use the same logic in loaders.

Ah ok I misunderstood your point initially. I think it make sense to expose it perhaps as an experimental flag too. (EDIT: looks like you already did it 😄)

bluwy
bluwy previously approved these changes Dec 20, 2022
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I quite like the idea of using loaders so putting my approval for now. But we may need a quick discussion before proceeding with this.

@cyco130
Copy link
Contributor Author

cyco130 commented Dec 21, 2022

I retracted the bit that exposes shouldExternalizeForSSR. Not necessary, see the updated original post.

@patak-dev patak-dev added this to the 4.1 milestone Dec 21, 2022
Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this in the last meeting, and we decided to move forward with this as an experimental option to let the ecosystem play with this new SSR mode. Let's review later if we should move it out of experimental once we have enough use cases in the wild.

@patak-dev patak-dev merged commit e781ef3 into vitejs:main Jan 18, 2023
futurGH pushed a commit to futurGH/vite that referenced this pull request Feb 26, 2023
@patak-dev
Copy link
Member

We are gathering feedback about this feature for future stabilization here:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: ssr p3-significant High priority enhancement (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants