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

fix: adapter-cloudflare-workers prerendering bug [w/ reproduction] #4467

Closed
wants to merge 6 commits into from

Conversation

f5io
Copy link
Contributor

@f5io f5io commented Mar 30, 2022

Hi there!

I noticed a slight discrepancy between adapter-cloudflare and adapter-cloudflare-workers. I need the ability to pass esbuild plugins into the adapter for some issues around running Stripe on cloudflare. Hopefully this small change is OK.

edit: In investigating further issues whilst attempting to run miniflare against a sveltekit build output, I discovered that there is an error in the way
prerendered files are resolved from the asset KV.

I am unable to complete a test run due to an error in adapter-netlify. I am also unable to complete a check due to an error in packages/kit/test/apps/amp.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Mar 30, 2022

🦋 Changeset detected

Latest commit: a5aa190

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

This PR includes changesets to release 1 package
Name Type
@sveltejs/adapter-cloudflare-workers 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

@f5io
Copy link
Contributor Author

f5io commented Mar 31, 2022

there also seems to be an issue with the resolution of prerendered pages for the cloudflare-workers adapter. it seems the getAssetFromKV's default mapRequestToAsset function doesnt quite cover the use-case properly.

i do have it working with some slight change to the manifest generation and by passing in a custom mapRequestToAsset function to getAssetFromKV however it is not yet part of this PR. happy to include it if needs be.

@dominikg
Copy link
Member

fyi, there are more pending changes to adapter-cloudflare-workers. #4276 there has been some discussion about kvAssetHandler in that PR too.

@f5io f5io changed the title feat: allow adapter-cloudflare-workers to take esbuild options fix: adapter-cloudflare-workers prerendering bug [w/ reproduction] Apr 1, 2022
@f5io
Copy link
Contributor Author

f5io commented Apr 8, 2022

thanks for pointing me in the direction of the existing work @dominikg. in doing a bit more research, ive found that the current release of the adapter simply does not work correctly for prerendered pages. is there any chance this could be seen as a hotfix that could be released rather than waiting on the migration to Module Workers?

@Rich-Harris
Copy link
Member

Closing in favour of #4626, which brings this up to date following #4276

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.

4 participants