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

feat(cloudflare): Exclude prerender and unused chunks from server bundle #222

Merged
merged 11 commits into from
Apr 18, 2024

Conversation

Fryuni
Copy link
Member

@Fryuni Fryuni commented Apr 5, 2024

Changes

I'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.

@Fryuni Fryuni self-assigned this Apr 5, 2024
Copy link

changeset-bot bot commented Apr 5, 2024

🦋 Changeset detected

Latest commit: fc0ff55

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

This PR includes changesets to release 9 packages
Name Type
@astrojs/cloudflare Minor
@test/astro-cloudflare-astro-dev-platform Patch
@test/astro-cloudflare-external-image-service Patch
@test/astro-cloudflare-no-output Patch
@test/astro-cloudflare-prerender-optimizations Patch
@test/astro-cloudflare-routes-json Patch
@test/astro-cloudflare-wasm Patch
@test/astro-cloudflare-with-solid-js Patch
@test/astro-cloudflare-wrangler-preview-platform 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

Copy link
Member

@alexanderniebuhr alexanderniebuhr left a 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.

Copy link
Member

@alexanderniebuhr alexanderniebuhr left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@alexanderniebuhr alexanderniebuhr left a 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 :)

.changeset/pink-rings-agree.md Outdated Show resolved Hide resolved
Copy link
Member

@ematipico ematipico left a 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(
Copy link
Member

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.

@alexanderniebuhr
Copy link
Member

!preview cf-no-prerender-chunks

Copy link
Contributor

Snapshots have been released for the following packages:

  • @astrojs/cloudflare@experimental--cf-no-prerender-chunks
Publish Log
🦋  warn ===============================IMPORTANT!===============================
🦋  warn Packages will be released under the experimental--cf-no-prerender-chunks tag
🦋  warn ----------------------------------------------------------------------
🦋  info npm info @astrojs/cloudflare
🦋  info npm info @astrojs/netlify
🦋  info @astrojs/cloudflare is being published because our local version (0.0.0-cf-no-prerender-chunks-20240412140922) has not been published on npm
🦋  warn @astrojs/netlify is not being published because version 5.2.0 is already published on npm
🦋  info Publishing "@astrojs/cloudflare" at "0.0.0-cf-no-prerender-chunks-20240412140922"
🦋  success packages published successfully:
🦋  @astrojs/cloudflare@0.0.0-cf-no-prerender-chunks-20240412140922
🦋  Creating git tag...
🦋  New tag:  @astrojs/cloudflare@0.0.0-cf-no-prerender-chunks-20240412140922
Build Log

> root@0.0.0 build /home/runner/work/adapters/adapters
> turbo run build --filter="@astrojs/*"

• Packages in scope: @astrojs/cloudflare, @astrojs/netlify, @astrojs/test-utils
• Running build in 3 packages
• Remote caching disabled
::group::@astrojs/netlify:build
cache miss, executing a68730fc97f3487c

> @astrojs/netlify@5.2.0 build /home/runner/work/adapters/adapters/packages/netlify
> tsc

::endgroup::
::group::@astrojs/cloudflare:build
cache miss, executing 12fa5edd1cdf35df

> @astrojs/cloudflare@0.0.0-cf-no-prerender-chunks-20240412140922 build /home/runner/work/adapters/adapters/packages/cloudflare
> tsc

::endgroup::

 Tasks:    2 successful, 2 total
Cached:    0 cached, 2 total
  Time:    3.63s 

@alexanderniebuhr alexanderniebuhr merged commit 8f312da into main Apr 18, 2024
8 checks passed
@alexanderniebuhr alexanderniebuhr deleted the feat/cloudflare-exclude-prerender-chunks branch April 18, 2024 05:38
@github-actions github-actions bot mentioned this pull request Apr 18, 2024
@ascorbic
Copy link
Contributor

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.

@Fryuni
Copy link
Member Author

Fryuni commented Jun 13, 2024

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

@alexanderniebuhr
Copy link
Member

@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.

@ascorbic
Copy link
Contributor

@alexanderniebuhr ooh, nice. Thanks for the pointer

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