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.

Sign up for GitHub

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: server islands regression in bundling #11702

Closed
wants to merge 3 commits into from
Closed

fix: server islands regression in bundling #11702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9690559
fix: server islands regression in bundling
ematipico Aug 14, 2024
3914b42
remove adapter from rollup inputs
ematipico Aug 14, 2024
07c4e8e
reduce code to server islands only
ematipico Aug 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/young-tigers-fry.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

Fixes a regression where an emitted hashed file contained characters that aren't allowed in some hosting platforms
2 changes: 1 addition & 1 deletion packages/astro/src/core/build/plugins/plugin-ssr.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ function vitePluginSSR(
}

const adapterServerEntrypoint = options.settings.adapter?.serverEntrypoint;
if (adapterServerEntrypoint) {
if (adapterServerEntrypoint && options.settings.config.experimental.serverIslands) {
inputs.add(adapterServerEntrypoint);
Copy link
Member Author

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not easily, no, you need to import node:crypto which doesn't work in non-Node environments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Only if globalThis.crypto isn't defined, which it would be on those runtimes

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

Expand Down
2 changes: 1 addition & 1 deletion packages/astro/src/core/build/static-build.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ async function ssrBuild(
return 'renderers.mjs';
} else if (chunkInfo.facadeModuleId === RESOLVED_SSR_MANIFEST_VIRTUAL_MODULE_ID) {
return 'manifest_[hash].mjs';
} else if (chunkInfo.facadeModuleId === settings.adapter?.serverEntrypoint) {
} else if (chunkInfo.facadeModuleId === settings.adapter?.serverEntrypoint && settings.config.experimental.serverIslands) {
return 'adapter_[hash].mjs';
} else if (
settings.config.experimental.contentCollectionCache &&
Expand Down
3 changes: 3 additions & 0 deletions packages/astro/test/test-adapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,9 @@ export default function ({
supportedAstroFeatures: {
serverOutput: 'stable',
envGetSecret: 'experimental',
hybridOutput: 'stable',
assets: 'stable',
i18nDomains: 'stable',
},
...extendAdapter,
});
Expand Down
Loading