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

Rollup 3.26.2 breaks Nitro's Deno builds #5067

Closed
AaronDewes opened this issue Jul 15, 2023 · 6 comments · Fixed by #5068
Closed

Rollup 3.26.2 breaks Nitro's Deno builds #5067

AaronDewes opened this issue Jul 15, 2023 · 6 comments · Fixed by #5068

Comments

@AaronDewes
Copy link

Rollup Version

3.26.2

Operating System (or Browser)

Linux

Node Version (if applicable)

18

Link To Reproduction

https://github.com/unjs/nitro

Expected Behaviour

Nitro works as usual with the Deno preset.

Actual Behaviour

unjs/nitro#1432.

Can be tested by running NITRO_PRESET=deno-deploy pnpm nitro build playground in the Nitro root. This fails with Rollup 3.26.2, but works if I downgrade to 3.26.1.

@AaronDewes AaronDewes changed the title Rollup 3.26.2 breaks Nitro Rollup 3.26.2 breaks Nitro's Deno builds Jul 15, 2023
@TrickyPi
Copy link
Member

TrickyPi commented Jul 15, 2023

There are some wrong options in thesrc/presets/deno-deploy.ts, an external module cannot be included in the manualChunks. You can fix this error by changing the manualChunks option to the following.

manualChunks: (id) => {
  if (id !== "https://deno.land/std/http/server.ts") return "index";
},

@AaronDewes
Copy link
Author

AaronDewes commented Jul 15, 2023

Okay, but the change you made regarding that in 3.26.2 was a breaking change (because the previous version worked without issues). In a patch release, breaking changes should be avoided in my opinion (and according to semver).

I would suggest to turn this error into a warning until Rollup 4.0 is released.

@lukastaegert
Copy link
Member

Ah, the issue is that external modules are actually passed to the manualChunks function. This should not be the case as those cannot be included. If we can fix this, then this would no longer be a breaking change except for the obvious case where someone explicitly included an external id.

@TrickyPi
Copy link
Member

Oh sorry folks, it's my fault. I submitted a PR to fix it.

@AaronDewes
Copy link
Author

Thank you!

@rollup-bot
Copy link
Collaborator

This issue has been resolved via #5068 as part of rollup@3.26.3. You can test it via npm install rollup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants