-
-
Notifications
You must be signed in to change notification settings - Fork 38
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(cloudflare): Exclude prerender and unused chunks from server bundle #222
feat(cloudflare): Exclude prerender and unused chunks from server bundle #222
Conversation
🦋 Changeset detectedLatest commit: fc0ff55 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Fryuni can you check node --test "packages/cloudflare/test/wasm.test.js"
, it's the test that fails 🤔
It is caused because I think because of the "prerendering bug" for endpoints. The hybrid.ts
in that project is prerendered, but still get's included in the pageMap here dist/_worker.js/index.ts
:
const _page2 = () => import('./chunks/hybrid_Cr-KnQBu.mjs');
const pageMap = new Map([
["../../../../../node_modules/.pnpm/astro@4.5.8_@types+node@18.17.8_typescript@5.2.2/node_modules/astro/dist/assets/endpoint/generic.js", _page0],
["src/pages/add/[a]/[b].ts", _page1],
["src/pages/hybrid.ts", _page2]
]);
And ./chunks/hybrid_Cr-KnQBu.mjs
has const page = () => import('./prerender_VBxqEc_S.mjs');
which breaks because ./prerender_VBxqEc_S.mjs
got correctly removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocked until upstream Astro limitation is fixed.
https://discord.com/channels/830184174198718474/845430950191038464/1225801545699037184
https://discord.com/channels/830184174198718474/845430950191038464/1225858482268405760
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some quick comments.
Once we have #227 merged into this one, all tests pass and everything is addressed, I'm happy to get this over the line :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good, but I am a bit surprised there are no comments explaining why this workaround is needed and how it works.
If next time a developer comes in here, they would have really hard time understanding the context of the logic.
(entry) => !prerenderImports.map((e) => e[1]).includes(entry) | ||
); | ||
for (const page of prerenderImports) { | ||
const importRegex = new RegExp( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add a comment that points to the actual place where Astro creates/writes these modules, so we know why it's there and how to fix it in case upstream decides to change things, even the name of a page.
!preview cf-no-prerender-chunks |
Snapshots have been released for the following packages:
Publish Log
Build Log
|
I'm looking at this in reference to withastro/astro#10552 and I'm wondering if this would be better to be in core rather than adapters. The same thing seems to happen for all adapters, and it shouldn't really be the responsibility of an adapter to clean up the build liek this. |
We started discussing how to better solve this in core. It is not on a proper thread, just scattered across the #contribute channel, but here is one message related to the topic |
@ascorbic take a look at withastro/astro#11245, I'm not totally sure if that fixes the withastro/astro#10552 issue, but it should make this workaround inside the adapter not needed anymore. |
@alexanderniebuhr ooh, nice. Thanks for the pointer |
Changes
_workers.js
folder/file bundle is too large #218I'm okay with putting this behind a flag (experimental or not), the wiring is quite trivial to make configurable.
Testing
I added an example of an offending project that includes all content collections in the server bundle even though those are only used in pre-rendered pages, causing unnecessary bloat.
Docs
I don't think this needs any docs changes, but happy to document it if others think it does.