fix: server islands regression in bundling #11702
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 07c4e8e The changes in this PR will be included in the next version bump. 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 |
|
!preview server-island-regression |
|
Snapshots have been released for the following packages:
Publish LogBuild Log |
ematipico
commented
Aug 14, 2024
| @@ -37,11 +42,6 @@ function vitePluginSSR( | |||
| inputs.add(getVirtualModulePageName(ASTRO_PAGE_MODULE_ID, pageData.component)); | |||
| } | |||
|
|
|||
| const adapterServerEntrypoint = options.settings.adapter?.serverEntrypoint; | |||
| if (adapterServerEntrypoint) { | |||
| inputs.add(adapterServerEntrypoint); | |||
There was a problem hiding this comment.
@matthewp what was the reason for adding the entry point to the list of inputs?
If there's a valid reason, I can restore this change, but it has be done in a different way. We would have to create a virtual module
There was a problem hiding this comment.
So that the adapter will be built to a separate module and get executed as one of the first things. With the addition of using crypto we need the Node adapter to run early because it polyfills in Node 18. Without this the adapter will be inlined into this main module and not execute in time. Here's the scenario:
import 'package-depends-on-crypto';
// Adapter code here
globalThis.crypto = ...This throws. because an imported dependency executes first. With this change however:
import './adapter.mjs';
import 'package-depends-on-crypto';This now works, because the adapter bundle is the first thing to execute, allowing it to polyfill anything it needs to.
There was a problem hiding this comment.
Isn't it just us that uses globalThis.crypto? Could we ponyfill it ourselves at the point of use instead of relying on a global polyfill?
There was a problem hiding this comment.
Not easily, no, you need to import node:crypto which doesn't work in non-Node environments.
There was a problem hiding this comment.
Only if globalThis.crypto isn't defined, which it would be on those runtimes
There was a problem hiding this comment.
Not sure I follow, globalThis.crypto is defined in ever environment (to my knowledge) other than Node 18. How would you suggest importing it without it causing bugs when Cloudflare / Deno do their bundling?
There was a problem hiding this comment.
I meant externalising it and importing it dynamically if it's undefined. Deno is fine because it supports node:crypto anyway. I don't know enough about how bundling works for Cloudflare to say if it would work.
There was a problem hiding this comment.
So we understand each other, what you're suggesting is this, right?
if(typeof crypto === 'undefined') {
await import('node:crypto');
}I'm pretty sure the bundle step for Cloudflare will complain about this. We could hide it or add an ignore comment or something possibly.
Part of my stubbornness here is that this is a correctness thing, the adapters are supposed to run first in order to polyfill the environment if they need to. In the past there was more of a need to do this as there was less alignment on globals. Today it's not the case as much as crypto is the only thing I'm aware of.
There was a problem hiding this comment.
Yeah, basically. That makes sense. I think the real fix here is to work out why rollup is unhappy with Netlify's entrypoint being external
|
I tested the reproduction against the latest commit, and it builds correctly |
matthewp
reviewed
Aug 14, 2024
db33864 to
07c4e8e
Compare
|
This will still break server islands on Netlify though, right? |
It's fine if server islands break, they are an experimental feature after all. But the initial reproduction didn't use server islands |
|
Closing in favor of #11709 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Closes #11700
I changed the condition that adds the server entrypoint to the rollup inputs, to only so when the server islands are enabled.
Testing
I will create a preview release and ask the user to test it.
The CI should pass
Docs
N/A