fix(cloudflare) "shared" wasm module imports#249
fix(cloudflare) "shared" wasm module imports#249alexanderniebuhr merged 6 commits intowithastro:mainfrom
Conversation
🦋 Changeset detectedLatest commit: b9e541c The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
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) { |
There was a problem hiding this comment.
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?
7efdd98 to
63338cb
Compare
63338cb to
ed76784
Compare
4d6ce4a to
e5cca34
Compare
e5cca34 to
59a7acd
Compare
59a7acd to
6da75aa
Compare
6da75aa to
e9d6bcd
Compare
|
@adrianlyjak Thank you so much for this contribution, I'll get to a review later today :) |
289d96c to
ff0644f
Compare
alexanderniebuhr
left a comment
There was a problem hiding this comment.
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 :)
| * @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 |
There was a problem hiding this comment.
This JSDoc is not correct, there is only one param which is an object.
Sounds good! I'll take a stab at it today, I can certainly at least remove the |
|
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 |
|
I just pushed up a change to remove the Looks like I could, at the very least, build up a map of the file names within |
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. |
|
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
fixes #250
Changes
.binimports, which conceptually are very similar to wasm. However I backed out of fully implementing this in order to keep this in scope of a bugfixTesting
Docs
No docs needed at this time, this is just a fix for a regression