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: Supports ES SSR build based on module type #2157

Closed
wants to merge 4 commits into from

Conversation

rschristian
Copy link
Contributor

@rschristian rschristian commented Feb 21, 2021

Summary

Closes #2152

When "type": "module" is set for a package, vite build --ssr will not build to CJS, but ESM

I'm not sure if it's worth adding a test for this? It would basically just be a test checking the the ability to read the package file and Rollup's ability to output ES. Let me know if I should though, I certainly can.

@koumaza
Copy link

koumaza commented Apr 15, 2021

+1
LGTM. For project using Svelte-kit, not being built as ES is fatal...

Copy link
Member

@GrygrFlzr GrygrFlzr left a comment

Choose a reason for hiding this comment

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

Could you rebase this against the main branch? There's been some shuffling around of some code and this section is now in the buildOuputOptions function.

const buildOuputOptions = (output: OutputOptions = {}): OutputOptions => {
return {
dir: outDir,
format: ssr ? 'cjs' : 'es',
exports: ssr ? 'named' : 'auto',

Yes, with the typo of "Output" -> "Ouput", oops. Would be great if you can also fix the typo. 😬

@rschristian
Copy link
Contributor Author

No worries! Rebased and corrected that typo

@GrygrFlzr GrygrFlzr added p2-nice-to-have Not breaking anything but nice to have (priority) and removed 🔍 review needed labels Apr 17, 2021
@patak-dev
Copy link
Member

@rschristian ssr externals are imported using require at this point:

* TODO right now externals are imported using require(), we probably need to

Could you check if this needs reworking as part of this PR?

@rschristian
Copy link
Contributor Author

I'll take a peek, yeah. Worked when I opened this PR, but that was a couple months ago now.

@rschristian
Copy link
Contributor Author

@patak-js Took me a bit to finagle the CJS SSR example into shape, but ESM server output is generated and everything seems to be correct / as I'd expect it with this PR.

Copy link
Member

@GrygrFlzr GrygrFlzr 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 PR with Evan:

  • This change needs a playground test that uses "type": "module", we could change one of the ssr playgrounds to match.
  • This needs to be configurable, as there are situations in which you may want to build an ESM project to a CJS output (e.g. serverless functions currently require a CJS entrypoint).
    ssr.format?: 'cjs' | 'es'
    This will override the SSR output. When the configuration option is not present, we can fall back to this PR's default of assuming "type": "module" = ESM.
  • We'll need to expose an SSR flag for the Vite config like so:
    export default ({ command, mode, ssr }) => {
    Where ssr is a boolean that is true in SSR mode. This also needs a test.

@rschristian
Copy link
Contributor Author

Will try to get that done next block of time I can find.

@rschristian
Copy link
Contributor Author

rschristian commented May 2, 2021

* This needs to be configurable, as there are situations in which you may want to build an ESM project to a CJS output (e.g. serverless functions currently require a CJS entrypoint).
  ```ts
  ssr.format?: 'cjs' | 'es'
  ```

@GrygrFlzr Are you looking for a ssr.format or ssrFormat? I'm assuming that's a typo and you meant the latter?

* We'll need to expose an SSR flag for the Vite config like so:
  ```js
  export default ({ command, mode, ssr }) => {
  ```
  
  Where `ssr` is a boolean that is true in SSR mode. This also needs a test.

Also not super sure how this relates to this PR in anyway? Seems like something that should be separate?

@rschristian
Copy link
Contributor Author

rschristian commented May 3, 2021

I added a new playground that uses "type": "module" as the test case here is super-simple and we don't really need to get involved with SSR in any way. Even having this behind SSR is a bit silly, as it's just post-build processing in native ESM, but I digress.

That being said, I couldn't figure out your test suite at all. Frankly I have no clue what's going on it in, way too much magic for my Sunday night brain. Essentially just need to make sure that vite build --ssr src/<something>.js on a module with "type": "module" outputs ESM and vite build --ssr src/<something>.js --ssrFormat cjs with "type": "module" outputs CJS.

Regardless, I'm on vacation for the next 2-ish weeks and won't be touching this for the duration. I'm not a user of Vite anyhow, and have 0 familiarity with the code base, so feel free to take this PR over and push to it or what not. You'll know better what you're looking for here.

@rschristian rschristian force-pushed the feat/ssrEsm branch 2 times, most recently from 9ad8bfe to 4d21b7a Compare May 3, 2021 04:46
@Shinigami92 Shinigami92 marked this pull request as draft May 3, 2021 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs test p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Allow SSR build to output ES
6 participants