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

Merged
merged 2 commits into from
Nov 22, 2024
Merged

Editor feedback #37

merged 2 commits into from
Nov 22, 2024

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
@guybedford guybedford merged commit eade394 into main Nov 22, 2024
1 check passed
@guybedford guybedford deleted the editor-feedback-1 branch November 22, 2024 18:00
@@ -1306,7 +1306,7 @@ contributors: Luca Casonato, Guy Bedford
<h1>
<ins>
GetModuleSourceModuleRecord (
_moduleSource_: an Object,
_specifier_: an Object,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably want to do the same to HostGetModuleSourceModuleRecord.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, posted #38.

@guybedford guybedford mentioned this pull request 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.

2 participants