This repository was archived by the owner on Aug 17, 2022. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 13
Nuance the handling of imports in ESM-integration #37
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
c032d7a
Nuance the handling of imports in ESM-integration as suggested in #32
lukewagner 4d2d5f0
Add note about Module reflection JS APIs
lukewagner 8fb91ab
Remove the special case for (global externref)
lukewagner c9d5995
Remove unused reference
lukewagner File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
Interesting point, this implies the possibility of namespace construction for all
WebAssembly.Instance
objects right? This might require an ECMA262 spec change / WebAssembly JS API change to reflect allWebAssembly.Instance
objects as having a Namespace Exotic Object representation that can be used / constructed.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's a good point. Technically, it would be the "exports object" (i.e.,
WebAssembly.Instance.exports
) that we might want to reframe as a Namespace Object. IIRC, I think this was something we explicitly considered when defining the JS API in the first place and it's the reason for the final freezing step of creating the exports object (so that we could change it to a Namespace Object in the future without breaking existing code that depended on mutating the POJO). The reason for not doing this sooner was, again IIRC, that Namespace Objects weren't implemented at the point in time we were shipping the JS API.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.
Yeah I think it would be important to unify the interface now to ensure consistency with these representations. Was the plan to transparently upgrade
"exports"
or introduce a new property? Since the exports object is frozen it seems that should be possible, but potential edge cases include the prototype difference of the Namespace Exotic object (eg noexports.hasOwnProperty()
so users doing that wouldn't work ifexports
changes to a namespace). It might still be safer to construct a newInstance.namespace
in the JS API. Would it be worth getting an issue going for that?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.
My preference would be to try to upgrade
exports
in-place and see if anything breaks (say, by having a browser ship the upgrade in a pre-release channel for a while as an experiment). Filing an issue makes sense but, to be clear, I think this would be a general JS API change, independent of Module Linking. Since it's ESM-related, maybe the right repo is ESM-integration?