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(vercel): CJS bundle fix #3051

Merged
merged 6 commits into from
Apr 15, 2022
Merged

fix(vercel): CJS bundle fix #3051

merged 6 commits into from
Apr 15, 2022

Conversation

JuanM04
Copy link
Contributor

@JuanM04 JuanM04 commented Apr 9, 2022

Changes

This fixes some issues when converting from ESM to CJS.

Testing

Docs

@changeset-bot
Copy link

changeset-bot bot commented Apr 9, 2022

🦋 Changeset detected

Latest commit: 76349ae

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@astrojs/vercel Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@FredKSchott
Copy link
Member

Thanks Juan! I don't feel confident enough to review this one, so leaving for @matthewp to take a look when he gets back on Tuesday!

@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Apr 12, 2022
@matthewp
Copy link
Contributor

This doesn't look like a Vercel-specific issue. This would happen during the static build.

The cause is this:

  • The superjson package has a module field.
    • Vite supports the module field.
    • Node.js does not support the module field.
  • The package does not have exports (which Node.js does support)

I can see a couple of possible solutions here:

  1. Disable module field resolution during SSR. This would force consistency.
  2. Bundle dependencies.

If this is going to be a common problem then we might want to do (1).

Maybe there are other solutions I'm not thinking about?

@JuanM04
Copy link
Contributor Author

JuanM04 commented Apr 13, 2022

This doesn't look like a Vercel-specific issue. This would happen during the static build.

I'm aware of that, it was some mismatch when doing ESM -> CJS. Since this integration is the only one doing it, I made a quick fix.

  1. Disable module field resolution during SSR. This would force consistency.
  2. Bundle dependencies.

I added the bundling of dependencies because I can't get Vercel's Node File Tracing working. But yes, the first options seems like a good idea. How can I do that?

@matthewp
Copy link
Contributor

I don't think there's a way to control this at the moment. There's this option https://vitejs.dev/config/#resolve-mainfields but it doesn't work in SSR currently.

I think the workaround for your own projects is to use noExternal: ['superjson'] which forces that module to be bundled.

Going to close this as we don't want to force CJS on everyone. Can continue discussing a general solution.

@matthewp matthewp closed this Apr 14, 2022
@natemoo-re
Copy link
Member

@matthewp Just wanted to flag that #3106 was closed in favor of this PR. I think we need some answer here.

@JuanM04
Copy link
Contributor Author

JuanM04 commented Apr 14, 2022

I don't know why this was closed. There was an error when Rollup generated a bundle in ESM and then esbuild converted to CJS because Vercel needs it. This PR makes Rollup generate CJS from the go and then uses esbuild to bundle the dependencies, which worked well for me.

The ideal case is not to bundle any dependency and having an __astro_entry.nft.json file with all the dependencies and their paths inside node_modules/ (which I didn't figure out yet). Reference. Nonetheless, bundling to CJS may let the developers install ESM dependencies and then be converted to CJS (although I'm not sure is esbuild does that).

Going to close this as we don't want to force CJS on everyone

This isn't forcing CJS on everyone, right? Only the people who has the Vercel adapter will have their output in CJS (mainly because Vercel doesn't support ESM)

@matthewp
Copy link
Contributor

matthewp commented Apr 14, 2022

Do you have any links about Vercel not supporting ESM? I couldn't find anything when I searched.

My hesitancy about this issue is a couple of things:

  1. The underlying problem is not caused by Vercel so fixing in Vercel leaves everywhere else still not working.
  2. Going from ESM to CJS is pretty dangerous. A CJS file cannot import any ESM files, for example. ESM can import CJS, but with caveats like the one you are experiencing here. Astro is entirely built with ESM, you author your code in ESM, the static build runs in ESM. To then suddenly go to CJS for prod seems pretty risky to me.

However, if Vercel really doesn't support ESM at all then that changes things a bit.

@JuanM04
Copy link
Contributor Author

JuanM04 commented Apr 14, 2022

@matthewp yeah, it isn't documented because they transpile from ESM to CJS internally anything inside a api/ folder. Nonetheless, whatever is inside an .outout/ folder has to be in CJS. Here is the evidence:

  1. When I was doing the adapter and the output was in ESM, Vercel threw an error.
  2. You can see the outputs of Next.js or SvelteKit with the Vercel adapter: both output CJS.
  3. You can see the issue node-fetch not working with Node.js Function vercel/vercel#7540. ESM-only packages doesn't work.

Vercel once supported it, but they reverted it because it caused some errors. You can see the PRs related to tue upgrading of @vercel/ncc inside the vercel/vercel repo.

@JuanM04
Copy link
Contributor Author

JuanM04 commented Apr 14, 2022

To then suddenly go to CJS for prod seems pretty risky to me.

I totally agree with you, but Vercel and Next.js do that anyways. SvelteKit only does it with the Vercel adapter and the in-development vite-plugin-vercel is in the same position.

@matthewp
Copy link
Contributor

matthewp commented Apr 14, 2022

Ok, let's reopen and continue the discussion.

@matthewp matthewp reopened this Apr 14, 2022
Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

Ok, thanks for your patience and for explaining everything @JuanM04!

@matthewp matthewp merged commit b0ba22c into main Apr 15, 2022
@matthewp matthewp deleted the fix/vercel-cjs-bug branch April 15, 2022 19:58
@github-actions github-actions bot mentioned this pull request Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants