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(cloudflare) "shared" wasm module imports #249

Merged
merged 6 commits into from
May 3, 2024

Conversation

adrianlyjak
Copy link
Contributor

@adrianlyjak adrianlyjak commented Apr 24, 2024

fixes #250

Changes

  • Updates build to always build in node mode, and then does a final string replacement after the rollup build is complete
  • Starts to skeleton out support for .bin imports, which conceptually are very similar to wasm. However I backed out of fully implementing this in order to keep this in scope of a bugfix

Testing

  • Updated tests to use a wasm module shared across SSG/SSR in order to reproduce and fix this bug
  • did manual testing on a complex site that makes use of wasm modules

Docs

No docs needed at this time, this is just a fix for a regression

Copy link

changeset-bot bot commented Apr 24, 2024

🦋 Changeset detected

Latest commit: b9e541c

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

/**
* Once prerendering is complete, restore the imports in the _worker.js to cloudflare compatible ones, removing the .mjs suffix
*/
async afterBuildCompleted(config: AstroConfig) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the ugliest part of the change. A quick final string replace file fixup so that the import path is what's needed for cloudflare. Its pretty well targeted based on the prebuilt replacements array, so at least its not a overly permissive string replace. This unfortunately does walk the entire tree of generated files though.

I'm not familiar enough with deeper internals to know whether there's a method for switching rendering out more appropriately (I noticed that a bunch of built code end up being replaced with 'noop' after the prerender stages is complete. Is there perhaps a better way to do this?

@adrianlyjak adrianlyjak changed the title Cloudflare special module imports fixes and improvements Cloudflare wasm module import fix, and binary module support Apr 25, 2024
@adrianlyjak adrianlyjak changed the title Cloudflare wasm module import fix, and binary module support Cloudflare wasm "shared" module import Apr 25, 2024
@adrianlyjak adrianlyjak changed the title Cloudflare wasm "shared" module import Fix cloudflare "shared" wasm module imports Apr 25, 2024
@adrianlyjak adrianlyjak force-pushed the fix-cloudflare-wasm branch 2 times, most recently from 4d6ce4a to e5cca34 Compare April 25, 2024 02:20
@adrianlyjak adrianlyjak marked this pull request as ready for review April 25, 2024 02:30
@adrianlyjak adrianlyjak changed the title Fix cloudflare "shared" wasm module imports fix(cloudflare) "shared" wasm module imports Apr 25, 2024
@alexanderniebuhr
Copy link
Member

@adrianlyjak Thank you so much for this contribution, I'll get to a review later today :)

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.

Overall this is a good idea, I would prefer to remove the bin part from this PR, since it will probably added by the following PR.

Additionally to this, I would also suggest to use vite's chunks e.g. generateBundle instead of walking the tree with fs.

I'm happy to commit to this PR and take it from here, or you can do the changes, let me know what you prefer :)

Comment on lines 22 to 23
* @param bin - if true, will load '.bin' imports as Uint8Arrays, otherwise will throw errors when encountered to clarify that it must be enabled
* @param wasm - if true, will load '.wasm?module' imports as Uint8Arrays, otherwise will throw errors when encountered to clarify that it must be enabled
Copy link
Member

Choose a reason for hiding this comment

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

This JSDoc is not correct, there is only one param which is an object.

@adrianlyjak
Copy link
Contributor Author

Overall this is a good idea, I would prefer to remove the bin part from this PR, since it will probably added by the following PR.

Additionally to this, I would also suggest to use vite's chunks e.g. generateBundle instead of walking the tree with fs.

I'm happy to commit to this PR and take it from here, or you can do the changes, let me know what you prefer :)

Sounds good! I'll take a stab at it today, I can certainly at least remove the bin part. The follow-on change ended up going a bit of a different direction once I separated it. Not super familiar with vite/rollup plugin dev, but I'll take a look at generateBundle, thanks!

@alexanderniebuhr
Copy link
Member

alexanderniebuhr commented Apr 25, 2024

This (https://github.com/withastro/adapters/blob/main/packages/cloudflare/src/utils/non-server-chunk-detector.ts#L31-L43) might help, so every chunk is one of the files and you can update it's code property to edit the content.

@adrianlyjak
Copy link
Contributor Author

I just pushed up a change to remove the .bin configuration. I took a quick look at generateBundle. The issue I'm seeing there is that seems to be called before prerendering occurs. I don't think I can replace the import at that point, since it will cause the node-based pre-rendering to fail again (with an unrecognized .wasm import).

Looks like I could, at the very least, build up a map of the file names within generateBundle, such that final edits after the prerender can just fix the specific needed files rather than traversing the whole file tree.

@adrianlyjak
Copy link
Contributor Author

Looks like I could, at the very least, build up a map of the file names within generateBundle, such that final edits after the prerender can just fix the specific needed files rather than traversing the whole file tree.

Pushed a change for this. This should make things more efficient at least. Although I'm wondering how the 'noop's get inserted in build files--I've noticed that the content of prerender files change before and after the pre-render. (I think that happens in astro core's build?). Was thinking that maybe if that's something I can hook into, it would be more appropriate.

@alexanderniebuhr
Copy link
Member

@adrianlyjak
Copy link
Contributor Author

adrianlyjak commented Apr 26, 2024

I think that happens in astro core's build?

https://github.com/withastro/astro/blob/b673bc850593d5af25793d0358c00797477fa373/packages/astro/src/core/build/static-build.ts#L398-L414

Ah, ok. Looks like that ends up taking a similar approach of just reading a targeted list of files and rewriting their contents on the fly from the file system, so I don't think I have any better solutions than the current implementation

…2. regex non-match issue with chunks that include "_" in their id
@alexanderniebuhr alexanderniebuhr merged commit 72fc8ac into withastro:main May 3, 2024
8 checks passed
@github-actions github-actions bot mentioned this pull request May 3, 2024
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.

"Shared" wasm module no longer builds
2 participants