Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: support WebAssembly (Wasm) imports in ESM modules #13505
feat: support WebAssembly (Wasm) imports in ESM modules #13505
Changes from 12 commits
95ef976
645db88
9985179
5d73f9a
bc6d38c
734bbc5
612d480
dcff11e
e524e40
08c8906
66c8312
6586a24
01f8d78
27d73b6
2aea674
79e0f40
0d4945e
26db3fd
1e6d75b
d4e9b73
ad32edd
397bcd1
976ef61
66a2312
7188086
26e7022
607fc0f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File source: https://github.com/hasharchives/wasm-ts-esm-in-node-jest-and-nextjs
Happy to replace this wasm with some canonical example file, but I don’t know any. Ideally, this file would export a function with arguments (mine is just
getAnswer = () => 42
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something from https://github.com/mdn/webassembly-examples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that seems reasonable 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 79e0f40, using
add(i32, i32)
from mdn/webassembly-examples.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have a
readFile
helper which caches the result. we should do the same here.readFile
currently reads as utf8, so maybe just a separatereadFileBuffer
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 0d4945e. I had to change the type for
_cacheFS
, which unfortunately spills outsidejest-runtme
. Happy to improve the implementation if you have any suggestions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite a few lines below are marked as changed because of extra indent. Hiding whitespace in diff viewer makes it a bit easier to see the real difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should these be put into
this. _esmoduleRegistry
as well?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve replaced
linkAndEvaluateModule
withloadEsmModule
in 26db3fd. Not 100% sure if this is right, but AFAIU,loadEsmModule
is a wrapper function that deals with_esmoduleRegistry
. If that’s not the best solution, feel free to push to my branch if the required change is not trivial.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm I should have kept
linkAndEvaluateModule
. UsingloadEsmModule
here can resultThis happens when Wasm imports refer back to a javascript file that calls it. E.g.:
loadEsmModule
returnswhich is then mistakenly passed to to
isWasm
instead of a string.I can try submiting a follow-up PR with a fix in a few days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#13608