fix(cloudflare) "shared" wasm module imports

adrianlyjak commented 2 months ago

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

changeset-bot[bot] commented 2 months ago

🦋 Changeset detected

Latest commit: b9e541c5c566fd79bd994712da4b8653a21c1e55

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

alexanderniebuhr commented 2 months ago

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

adrianlyjak commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

adrianlyjak commented 2 months ago

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

withastro / adapters