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 ssr.format to force esm output for ssr #6812

Merged
merged 1 commit into from May 29, 2022

Conversation

manucorporat
Copy link
Contributor

Description

Add new config: ssr.format to force an specific module format of the SSR build

Additional context

Deno, Cloudflare and other platforms might run SSR code, but it needs to be described in ESM format.

This PR solves the same issue as #2157, but takes a more explicit approach, not using package.json.

Closes #2152

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.

@Niputi Niputi added feat: ssr p2-nice-to-have Not breaking anything but nice to have (priority) labels Feb 8, 2022
@bluwy bluwy added the YAO Yet another option... label Feb 8, 2022
@bluwy
Copy link
Member

bluwy commented Feb 8, 2022

ESM output can be set today via build.rollupOptions.output.format: "esm". SvelteKit is currently doing so. I don't think we need a new option for this then?

@mquandalle
Copy link

@bluwy I wasn't aware of this option, and I guess I'm not the only one looking at the number of 👍 here.

I have an app that is fully ESM except for a single prerender.cjs script because I thought the ssr bundle had to be in CJS for technical reasons.

https://github.com/betagouv/mon-entreprise/blob/dd20ce09c384c95c307dffd9f4e6149074e2c566/site/prerender.cjs#L2-L5

Maybe this need to be better documented in the SSR section?

@bluwy
Copy link
Member

bluwy commented Feb 9, 2022

Yes, we should definitely document the build.rollupOptions.output.format option. Not sure why there's so many 👍

@manucorporat Are you fine if we close this and document the option instead?

@guifromrio
Copy link

Great! I was working with Manu when he spotted this apparent problem. I can confirm the config works and I was able to undo my hack (guifromrio/qwik-app-deno-example@6bf27a5#diff-edab6281f770861b1b43fd3fb256f40626f5d9fc79e3feccdac8c3e594241eb1L2)

So, yes, documenting this might help. "In case your runtime requires esm, use this option"

@patak-dev
Copy link
Member

I think that for the most common use cases, we shouldn't need to reach into rollupOptions. So ssr.format still may be a good addition, we need to properly review how it interacts with ssr.target. If target is webworker, format needs to be es. Maybe we could throw an error if the user sets it to cjs.

@bluwy
Copy link
Member

bluwy commented Feb 10, 2022

I've been thinking about this recently and discussed with patak. I've concluded that ssr.format would be useful too due to the reasons patak highlighted. And if one Vite config is to be used to output client and server build, setting rollupOptions won't be simple. Maybe we can have 3 targets instead? node, node-esm, and webworker

@guifromrio
Copy link

I would just not call is node-em, but simply esm since Deno, for example, is a runtime that will use it.

@patak-dev
Copy link
Member

We discussed the PR in the last team meeting.
The ssr.target should be enough here, but it is currently not properly working. If ssr.target is webworker the output format should be es instead of cjs. For targeting Deno, looks like using webworker once the output format is fixed should also work. So we don't need another option.

  1. We should fix ssr.target: 'webworker' to work out of the box for Cloudflare workers
  2. We should then check if the same target works in Deno
  3. We should then decide if the value should be renamed to something like ssr.target: 'node' | 'web', (or maybe 'browser'?).

@bluwy
Copy link
Member

bluwy commented Feb 14, 2022

From the code, one unique part of webworker is that the define plugin would replace process.env. etc as ({}). as process doesn't exist in non-node environments. So we can't actually use webworker if we want to output ESM for Node though.

if (!ssr || config.ssr?.target === 'webworker') {

@patak-dev
Copy link
Member

Good point @bluwy, I think the idea would be that when ssr.target is node, then the format of the SSR bundle would be defined by the presence of "type": "module" in package.json. If it is present, as in Svelte, then the SSR bundle will be es.

I think this covers all current cases, no? The last time we talked about the previous PR we ended up deciding to include ssr.format because we didn't have ssr.target. We could still add ssr.format (maybe better than having two more targets node-cjs, node-es), if there is a case where the automatic target using "type": "module" is still not enough (I imagine that could happen if you target some deploy platforms that don't support ESM? Is that still a thing?)

@bluwy
Copy link
Member

bluwy commented Feb 15, 2022

Yeah the automatic type: module detection for node target would be a nice default. I was also worried that there are usecases out there than needs to deviate from the defaults, and if so there isn't a straightforward way unless we point towards build.rollupOptions.output.format (maybe it isn't too bad). I'm not sure if we need ssr.format though since webworker would always be es, which can be conflicting.

So perhaps we could go forward with:

  1. Target webworker is always in es format
  2. Target node relies on type: module to determine format
  3. Other usecases would use build.rollupOptions.output.format

@ayuhito ayuhito mentioned this pull request Apr 30, 2022
4 tasks
@bluwy bluwy mentioned this pull request May 13, 2022
4 tasks
@benmccann
Copy link
Collaborator

I think it would be confusing to have two ways of doing thing. It may be better to simply highlight the existing option in the documentation

@patak-dev
Copy link
Member

I'll merge this PR, as we're going to use the same option in:

Check that PR for reference on related work around defaulting Vite to use ESM for the SSR build by default. We're also removing the externalization heuristics in v3.

@patak-dev patak-dev merged commit 337b197 into vitejs:main May 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: ssr p2-nice-to-have Not breaking anything but nice to have (priority) YAO Yet another option...
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Feature: Allow SSR build to output ES
7 participants