Skip to content
This repository was archived by the owner on Feb 10, 2025. It is now read-only.

feat(cloudflare): Add support for txt and bin cloudflare specific module types#251

Merged
alexanderniebuhr merged 3 commits intowithastro:mainfrom
adrianlyjak:feat-cloudflare-bundling
Jun 7, 2024
Merged

feat(cloudflare): Add support for txt and bin cloudflare specific module types#251
alexanderniebuhr merged 3 commits intowithastro:mainfrom
adrianlyjak:feat-cloudflare-bundling

Conversation

@adrianlyjak
Copy link
Copy Markdown
Contributor

@adrianlyjak adrianlyjak commented Apr 25, 2024 •
edited
Loading

A follow on feature to #249

Changes

  • In addition to the existing wasm imports, this adds support for loading .bin and .txt files within cloudflare pages functions
  • renamed wasm related code and config options to module-loader to clarify that this supports multiple formats
    • renamed wasmModuleImports to cloudflareModules, retaining the previous so that this is non-breaking.
  • supports plain .wasm imports instead of only .wasm?module imports to be consistent with cloudflare pages
  • Modified the behavior to default to allowing wasmModuleImports by default. Initially we'd decided to keep this opt-in so as to keep the hacky branching from breaking the build: feat(@astrojs/cloudflare): Add support for wasm module imports astro#8542 (comment). There's no more branching, so now the vite plugin here is doing close to nothing except for string comparisons if there's no matching import, so it seems safe to me now. (You'd effectively be opting in by importing the right file format)
  • long term structure for the implemenation is such that it in the future it can easily be extended to support custom file extensions
    • either based on wrangler.toml rules (if cloudflare pages gets support for that, right now it seems to be workers only)
    • or perhaps the plugin could rewrite the extensions to one of the supported data types (CompiledWasm -> .wasm, Data -> .bin, or Text -> .txt)

Testing

  • Added additional test cases for each format to the wasm (now module-loader) test suite

Docs

I changed the name from wasmModuleImports to cloudflareModules in the cloudflare config. The old parameter is still available, but marked as deprecated. Any opinions around terminology here? Most everything I come up with ends up sounding generic, IMO, its hard to name this feature, so I was just going off terminology that cloudflare uses https://developers.cloudflare.com/pages/functions/module-support/.

What's the process for updating documentation? Should I make an associated merge request in the docs repo? https://github.com/withastro/docs/blob/main/src/content/docs/en/guides/integrations-guide/cloudflare.mdx

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 25, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: f078e77

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-wasm Patch
@test/astro-cloudflare-no-output Patch
@test/astro-cloudflare-prerender-optimizations Patch
@test/astro-cloudflare-routes-json 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

@alexanderniebuhr alexanderniebuhr self-requested a review April 25, 2024 15:34
@adrianlyjak adrianlyjak force-pushed the feat-cloudflare-bundling branch 3 times, most recently from e52355b to a0a031e Compare April 25, 2024 23:50
@adrianlyjak adrianlyjak marked this pull request as ready for review April 26, 2024 00:05
@adrianlyjak
Copy link
Copy Markdown
Contributor Author

adrianlyjak commented Apr 26, 2024

better diff here: adrianlyjak/withastro-adapters@fix-cloudflare-wasm...adrianlyjak:withastro-adapters:feat-cloudflare-bundling

@adrianlyjak adrianlyjak force-pushed the feat-cloudflare-bundling branch from a0a031e to cb112aa Compare April 26, 2024 16:41
@alexanderniebuhr
Copy link
Copy Markdown
Member

alexanderniebuhr commented Apr 28, 2024

@adrianlyjak yeah a docs pr to the docs repo is needed :)

alexanderniebuhr
alexanderniebuhr suggested changes Apr 28, 2024
View reviewed changes
Copy link
Copy Markdown
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.

@adrianlyjak Thanks for the contribution, very happy to have it ❤️
As already said, this needs a companion docs PR in the docs repo. (fyi @sarah11918)

I personally like that you took the step to still support the old flag, so we can ship this as a minor.

adrianlyjak pushed a commit to adrianlyjak/withastro-docs that referenced this pull request May 7, 2024
Updated docs for withastro/adapters#251
1146421
@adrianlyjak adrianlyjak mentioned this pull request May 7, 2024
Merged
adrianlyjak pushed a commit to adrianlyjak/withastro-docs that referenced this pull request May 7, 2024
Updated docs for withastro/adapters#251
d52d4e8
adrianlyjak pushed a commit to adrianlyjak/withastro-docs that referenced this pull request May 7, 2024
Updated docs for withastro/adapters#251
ae857c8
@adrianlyjak adrianlyjak changed the title feat(cloudflare): Add support for more "bundling" formats feat(cloudflare): Add support for txt and bin cloudflare specific module types May 7, 2024
@adrianlyjak adrianlyjak force-pushed the feat-cloudflare-bundling branch from cb112aa to 90ee0d3 Compare May 7, 2024 20:31
adrianlyjak pushed a commit to adrianlyjak/withastro-docs that referenced this pull request May 7, 2024
Updated docs for withastro/adapters#251
8104288
@adrianlyjak
Copy link
Copy Markdown
Contributor Author

adrianlyjak commented May 7, 2024

@alexanderniebuhr @sarah11918 initial merge request for doc updates is here withastro/docs#8211. Let me know if you have any feedback!

@adrianlyjak adrianlyjak force-pushed the feat-cloudflare-bundling branch from 90ee0d3 to 64fac3f Compare May 7, 2024 20:36
@alexanderniebuhr
Copy link
Copy Markdown
Member

alexanderniebuhr commented May 7, 2024

Thanks for the update. Will review both PR's ;)

alexanderniebuhr
alexanderniebuhr reviewed Jun 7, 2024
View reviewed changes
@alexanderniebuhr alexanderniebuhr merged commit b826675 into withastro:main Jun 7, 2024
alexanderniebuhr added a commit to withastro/docs that referenced this pull request Jun 7, 2024
@adrianlyjak @sarah11918 @alexanderniebuhr
Update cloudflare adapter docs for new import types (#8211)
e39978f 
* Updated docs for withastro/adapters#251

* fix spelling/grammar in cloudflare modules documentation

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>

---------

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Co-authored-by: Alexander Niebuhr <alexander@nbhr.io>
@github-actions github-actions bot mentioned this pull request Jun 7, 2024
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@alexanderniebuhr alexanderniebuhr alexanderniebuhr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

docs pr pkg: cloudflare

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@adrianlyjak @alexanderniebuhr