Skip to content
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

Editor feedback #37

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Editor feedback #37

wants to merge 2 commits into from

Conversation

guybedford
Copy link
Collaborator

@guybedford guybedford commented Nov 22, 2024

Integrates the feedback from @michaelficarra in #26 (comment).

  • EvaluateImportCall step 9.b.i: don't say "is not equal to", just say "is not"; see https://github.com/tc39/ecma262/wiki/Editorial-Conventions#comparisons.
  • GetModuleSourceModuleRecord: wouldn't it be more appropriate to name this parameter specifier? Isn't it still conceptually a specifier?
  • GetModuleSourceModuleRecord step 5: "is equal to" -> "is"
  • Source Text Module Records [[SourceText]] field: "ECMAScript source text" is probably a better type. Also I wouldn't include the optimisation note for hosts.
  • Source Text Module Records [[ModuleSource]] field: like the above rows, we should describe when it's expected to be empty
  • ModuleSourcesEqual step 2: "is equal to" -> "is"
  • The ModuleSource constructor: I don't think this is needed: "will throw an error when invoked, where support for dynamic construction may be added in future"
  • "For Module Records that implement a normal completion" -> "For Module Records that return a normal completion"

Where is ModuleSourcesEqual called? Why is it on abstract module records and not just source text module records (or better yet, just an AO) if we know it'll only be called on them?

ModuleSourcesEqual is a helper function that is necessary for performing identity checks within HostLoadImportedModule. Since JS and Wasm modules have independent definitions of this equality, this allows us to implement it for JS, then Wasm can implement it on its own concrete module record, and the HTML spec just needs to call the more general ModuleSourcesEqual function. We would need to migrate keying into ECMA-262 for this to be called from ECMA-262 directly. Removing the helper function pushes out more complexity into HTML, which is an option, but when I started writing this as an HTML function it just fitted much better here.

I don't love the not-a-source enum name, but we can bikeshed that during integration

Sure!

@guybedford guybedford changed the title Editor feedback 1 Editor feedback Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant