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
Support Wasm files that import JS resources #13608
Support Wasm files that import JS resources #13608
Changes from 4 commits
942188c
1c58803
a7eac68
ea9133e
65223df
5d4a115
582a86b
7dc9940
f115872
98116ea
15708bc
797437b
2de0f1f
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.
Unused import?
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.
It looks so, yes. All three files in this folder are auto-generated and I’ve decided to keep them as is. This will be helpful in the future if we need to re-generate the example again.
Maybe this like has its meaning actually. It makes sure that the wasm file is imported before JS, which may affect the result.
index_bg.wasm
andindex_bg.js
import stuff from each other,bg
stands forbindgen
.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.
It's still added to the cache, right?
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 don’t know for sure unfortunately. There are calls to
this._esmModuleLinkingMap
→get/set
insidelinkAndEvaluateModule
but I don’t know what that means. All I know is thatloadEsmModule
cannot be used here – tests don’t pass.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 need to make sure the module is checked for in
_esModuleRegister
or what it's called. And then re-used or added afterwardsThere 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.
AFAIU, the module already ends up in
_esModuleRegister
forwasm-bindgen
packages. We have this import flow:Because
index_bg.js
is imported fromindex_bg.js
,index_bg.js
ends up in the cache registry by then. But because of circular deps, callingloadEsmModule
instead oflinkAndEvaluateModule
fails because it returns an object withstatus: 'linking'
instead of a string path.It’s possible to imagine a Wasm file that imports from some other JS file. I don’t know what will happen in this case TBH. Perhaps we can focus on fixing a practical problem in this PR and leave this theoretical scenario for later. Wasm support is still experimental anyway.
Feel free to push changes if you can think of any! The key added value of this PR so far is a new test case, not the fix.