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

workaround prerender in pageMap #227

Merged

Conversation

alexanderniebuhr
Copy link
Member

@alexanderniebuhr alexanderniebuhr commented Apr 7, 2024

we need to plugins so we can use generateBundle twice. Any other hook doesn't have file names with hashes already, and using writeBundle would need fs operation, which I think is worse.

regex is scary, but I'm not sure if it is worth to create an ast and convert back to string again

Copy link

changeset-bot bot commented Apr 7, 2024

⚠️ No Changeset found

Latest commit: 9277080

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Member

@Fryuni Fryuni left a comment

Choose a reason for hiding this comment

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

I don't get why this can't be done with a single plugin. Can't you do one loop after the other in the same plugin?

packages/cloudflare/src/index.ts Outdated Show resolved Hide resolved
packages/cloudflare/src/index.ts Outdated Show resolved Hide resolved
@alexanderniebuhr
Copy link
Member Author

@Fryuni Updated the logic, now the regexp should be much safer. wdyt?

@bluwy
Copy link
Member

bluwy commented Apr 11, 2024

If this workarounds some issue with pre-rendering currently, then the code seems fine to me. But I think given the workarounds needed so far, we should prioritize making prerendering more robust in Astro core.

@Fryuni
Copy link
Member

Fryuni commented Apr 11, 2024

Seems good now. Well, as good as a workaround using regex can be 😅

If this workarounds some issue with pre-rendering currently, then the code seems fine to me. But I think given the workarounds needed so far, we should prioritize making prerendering more robust in Astro core.

Pre-rendering itself is not a problem. The fact that Astro builds a single bundle for pre-rendering and server rendering and considers the other as (hopefully) unreachable code is the problem. If you have a single server route, you need to upload the code to generate all your routes, which can hit the target platform's source size limits.

This was noticed first on Cloudflare because it has the smallest limit for the free tier, but Vercel, Netlify, and others are also subject to this problem if you have a big enough project.

@alexanderniebuhr
Copy link
Member Author

@Fryuni @bluwy Both of you are right. This is not really a prerendering issue in it's core, but we have discovered other issues which are also caused by prerendering behavior. So prioritizing this in core makes total sense to me.

@alexanderniebuhr alexanderniebuhr merged commit a6b5f78 into feat/cloudflare-exclude-prerender-chunks Apr 11, 2024
8 checks passed
@alexanderniebuhr alexanderniebuhr deleted the workaround-core branch August 6, 2024 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants