feat(@astrojs/cloudflare): Add support for wasm module imports #8542

adrianlyjak · 2023-09-13T15:28:25Z

Changes

Adds vite and esbuild plugins for loading web assembly modules to fix Cloudflare adapter doesn't support WASM, despite natively supporting WASM modules #8541

Testing

automated tests were added that imports web assembly modules from the 3 different possible directory structures
manual testing can be done be importing a simple .wasm file and using it. Wasm files should be able to be imported from node_modules as well.
Refactors cloudflare wrangler tests to be more reliable by dynamically rotating ports, and retrying timeouts. Removes kill-port dependency. I attempted to removes skips for flaky startup (I wasn't encountering any flakiness after the port fix locally), however the windows and node 20 environments seem to still consistently fail on 1 or 2 wrangler cli tests. Instead I refactored the skip behavior to be a little simpler to opt-in or out.

Docs

Updated README for cloudflare adapter with small section on wasm modules.

changeset-bot · 2023-09-13T15:28:30Z

🦋 Changeset detected

Latest commit: 6a806ef

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

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

adrianlyjak · 2023-09-13T22:21:46Z

packages/integrations/cloudflare/src/index.ts

-						},
-					});
+					//
+					// Sadly, this needs to build esbuild for each depth of routes/entrypoints independently so that relative


Any ideas how this could be improved for functionPerRoute mode? The problem is that the esbuild is effectively creating the exact same bundle and duplicating, and now the bundle needs to have relative imports, so the bundles need to be different depending on how deep they are in the tree

IMHO wasm should be an opt-in functionality

@alexanderniebuhr could you explain why, please? I'm curious :)

I'll add my 2 cents. It would be nice if this was zero configuration. Its not really "changing" behavior like other configs, rather its adding behavior that the user would expect to work given what works in cloudflare.

That being said, this multiple esbundle depending on depth of the entrypoint is a bit of a hack, and if it stays that way, adding a configuration to opt-in to wasm loading for at least the function per route case makes sense to me.

@alexanderniebuhr were your reservations around this hack or something else?

About the hack, In my development, I wasn't able to specify a different import pattern for the different entrypoints using its onResolve callback. But it seems like there must be some other better way to do it. I was just hoping that someone might have better insight. Looking deeper at the esbuild plugin documentation, I might be able to do something with the onEnd callback to modify the imports within each entry

@ematipico @adrianlyjak let me explain!

I'm genuinely excited about this feature and would love to see it enabled by default in the long run. 🚀

However, I've got a few thoughts:

I'm a bit hesitant about the entryPathsGroupedByDepth solution. I believe there might be a more efficient approach out there. I'm already thinking about it but need some more time, to fully understand the requirements.

However I don't think it should block this PR - we should iterate on it in a future PR!

Given that wasm isn't a typical use-case for majority of users, my suggestion would be to make this feature opt-in for now. This way, we ensure we don't inadvertently degrade the experience for users without wasm - which we test for, but it still feels a bit hacky.

I also haven't dived deeper into a thorough code review yet. So I don't really know if the current solution is that hacky or fine. I was just going by my initial gut feeling and the comment provided by @adrianlyjak.

Once we've tested this with real-world use cases & optimized the adapter to reduce code branches (I'm working on that), I would consider making the default true, but still leave the flag intact (for user choice).

This mirrors how I usually approach runtime support: opt-in first & switching to default once I'm sure it works for 95% of users.

If we decide to invest the time to find a better solution for the depth issue and include it in this PR, I would love to see it enabled by default. I was just going to suggest: "Ship this first - deal with improvements later."

@adrianlyjak I'm keen to assist and offer a fresh perspective. While I haven't fully grasped all the issues yet, I'll delve deeper into your implementation to better understand the challenges you've described. I believe there might be some hooks we can leverage to streamline the process. To reiterate my earlier point, I feel we should merge this PR as it stands and focus on refining it in subsequent iterations.

What are your thoughts, @ematipico?

The only further idea to support both would be to use base64 in the vite build always, and then somehow re-interpret an wasm import within the worker esbuild.

I ended up doing this. It seems to work well and not be too complicated. Main complaint is that there's loosely coupled and hard to follow assumptions between the 2 build stages

Given the long discussion and the technical details that surfaced, I think we should keep the feature opt-in, so we can discuss them offline (discord, github issue, etc.) I want @adrianlyjak 's contribution to be merged ASAP, but we shouldn't rush into "enabled by default".

Currently the feature is opt-in. What's the process for merging? Or do you want further code review? I see this is starting to accumulate conflicts, and I'm hoping to not have to keep up with those. I'll rebase this for the time being (excited about the node compatibility that's conflicting here 😄 )

This PR looks good to me, amazing work! 🚀

In general the process is some more reviews, including one of docs team (maybe uncomment the review request from the PR template.) Once they all approve, merging will be handled by one of the maintainers :)

…tic site generation as well

sarah11918

Thanks so much for these docs contributions, @adrianlyjak! I found them super easy to follow and I think they'll be helpful to people using the integration! 🙌

I just had some suggestions for your consideration to adjust them a tiny bit so they would be more in line with how we'd normally write our Astro documentation. See what you think and as I always have to say, I'm just editing for style and structure. It's your job to make the statements true! 😉

packages/integrations/cloudflare/README.md

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

ematipico

Great work!

sarah11918

Looking good @adrianlyjak ! Just two tiny suggestions, then we're good to go!

packages/integrations/cloudflare/README.md

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

sarah11918

Love to approve great docs! 🥳

github-actions bot added the pkg: integration Related to any renderer integration (scope) label Sep 13, 2023

adrianlyjak force-pushed the main branch 11 times, most recently from 20a0905 to 102f617 Compare September 13, 2023 23:08

github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Sep 14, 2023

adrianlyjak force-pushed the main branch 9 times, most recently from 0afaaa1 to 758f9f4 Compare September 14, 2023 03:21

adrianlyjak marked this pull request as ready for review September 14, 2023 03:22

adrianlyjak requested a review from a team as a code owner September 14, 2023 03:22

adrianlyjak force-pushed the main branch from 758f9f4 to 6706651 Compare September 14, 2023 03:24

adrianlyjak commented Sep 14, 2023

View reviewed changes

adrianlyjak changed the title ~~fix(cloudflare): Add support for wasm module imports for #8541~~ fix(cloudflare): Add support for wasm module imports Sep 14, 2023

adrianlyjak force-pushed the main branch from 6706651 to 84d1925 Compare September 15, 2023 03:51

adrianlyjak changed the title ~~fix(cloudflare): Add support for wasm module imports~~ feat(@astrojs/cloudflare): Add support for wasm module imports Sep 17, 2023

adrianlyjak force-pushed the main branch 11 times, most recently from 26d3fbe to a925450 Compare September 17, 2023 20:25

adrianlyjak added 3 commits September 21, 2023 15:33

feat(cloudflare): Add support for wasm module imports for withastro#8541

68c5b0d

feat(cloudflare): Make wasm module imports conditional

dc19b0d

cloudflare wasm: add more complex test scenarios. Support wasm in sta…

92c9e7c

…tic site generation as well

adrianlyjak force-pushed the main branch from a925450 to 92c9e7c Compare September 21, 2023 19:33

matthewp approved these changes Sep 21, 2023

View reviewed changes

sarah11918 reviewed Sep 21, 2023

View reviewed changes

Apply documentation edits from code review

7ee3948

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

adrianlyjak force-pushed the main branch from 377e297 to 7ee3948 Compare September 22, 2023 04:22

adrianlyjak requested a review from sarah11918 September 22, 2023 04:34

ematipico approved these changes Sep 22, 2023

View reviewed changes

sarah11918 reviewed Sep 22, 2023

View reviewed changes

packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved

packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved

Apply sentance edits from code review to cloudflare Wasm readme

6a806ef

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

adrianlyjak requested a review from sarah11918 September 22, 2023 14:01

sarah11918 approved these changes Sep 22, 2023

View reviewed changes

ematipico merged commit faeead4 into withastro:main Sep 22, 2023
13 checks passed

astrobot-houston mentioned this pull request Sep 22, 2023

[ci] release #8635

Merged

adrianlyjak mentioned this pull request Apr 25, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(@astrojs/cloudflare): Add support for wasm module imports #8542

feat(@astrojs/cloudflare): Add support for wasm module imports #8542

adrianlyjak commented Sep 13, 2023 •

edited

Loading

changeset-bot bot commented Sep 13, 2023 •

edited

Loading

adrianlyjak Sep 13, 2023

alexanderniebuhr Sep 14, 2023

ematipico Sep 15, 2023

adrianlyjak Sep 15, 2023

alexanderniebuhr Sep 15, 2023 •

edited

Loading

alexanderniebuhr Sep 17, 2023

adrianlyjak Sep 17, 2023

ematipico Sep 18, 2023

adrianlyjak Sep 21, 2023

alexanderniebuhr Sep 21, 2023 •

edited

Loading

sarah11918 left a comment

ematipico left a comment

sarah11918 left a comment

sarah11918 left a comment

feat(@astrojs/cloudflare): Add support for wasm module imports #8542

feat(@astrojs/cloudflare): Add support for wasm module imports #8542

Conversation

adrianlyjak commented Sep 13, 2023 • edited Loading

Changes

Testing

Docs

changeset-bot bot commented Sep 13, 2023 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderniebuhr Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderniebuhr Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

sarah11918 left a comment

Choose a reason for hiding this comment

ematipico left a comment

Choose a reason for hiding this comment

sarah11918 left a comment

Choose a reason for hiding this comment

sarah11918 left a comment

Choose a reason for hiding this comment

adrianlyjak commented Sep 13, 2023 •

edited

Loading

changeset-bot bot commented Sep 13, 2023 •

edited

Loading

alexanderniebuhr Sep 15, 2023 •

edited

Loading

alexanderniebuhr Sep 21, 2023 •

edited

Loading