Stage 3 reviews

[nicolo-ribaudo]

This proposal reached conditional consensus for Stage 3 during the March 2023 TC39 meeting.

The consensus is conditional on #133 being merged, and on Stage 3 reviews. This issue tracks the review status.

• @jridgewell (review: Stage 3 reviews #137 (comment), pr: Fixes for jridgewell's review #139)
• @ljharb (review: Stage 3 reviews #137 (comment) and Stage 3 reviews #137 (comment), pr: ljharb's review #141)
• @tc39/ecma262-editors (Normative: Add Import Attributes ecma262#3057)

[jridgewell]

• EvaluateImportCall
  • I can't find EnumerableOwnPropertyNames anywhere, did you mean EnumerableOwnProperties?
  • "Sort attributes by the code point order of…", I don't see this phrase for sorting anywhere else in the spec so I'm not sure if it's valid.
• AllImportAttributesSupported
  • "For each Record attribute of attributes" should list the Record properties Record { [[Key]], [[Value]] }, or use "For each ImportAttribute Record…"
• Static Semantics: ModuleRequests
  • I don't see a pattern like "Let specifier be SV of the ModuleSpecifier contained in FromClause" anywhere else. Is this valid?
• ModuleRequest and ImportAttribute Records
  • We use "same [[Attributes]]" all over (for equality in ([[Specificer]], [[Attributes]]) paris), but we don't define its equality when using "same", only when using AttributesEqual.
• HostLoadImportedModule
  • The editorial note "There are three possible ways to handle multiple imports of the same module with different valid attributes:" is biased towards the Race semantics (which IMHO is the wrong choice) because the other semantics choices have caveats. Let's either add a caveat that Race will behave incorrectly in some scenarios, or remove the caveats from the other choices.
• AttributesEqual
  • Nit: I would split the looping If statement to do an early return if [[Key]] matches but [[Value]] doesn't.
• Static Semantics: WithClauseToAttributes
  • "Let key be StringValue of AttributeKey.", StringValue isn't defined on AttributeKey: StringLiteral.
  • Ditto for "[[Value]]: StringValue of StringLiteral"

[nicolo-ribaudo]

Thanks @jridgewell! I opened #139 for your review.

The only point I'm not sure how to solve is "Sort attributes by the code point order of…". There is already a step in the spec that sorts a list (step 6 of ModuleNamespaceCreate), and it does so by referring to %Array.prototype.sort%. However, that strategy doesn't work in this case because we are not sorting a list of values, but a list of records with an order based on one of their fields. I could write a separate sorting AO, but it feels overkill for the use case.

[bakkot]

"code point order" is kind of a tricky concept here, because strings in JS are lists of UTF16 code units, not of code points - so, for example, does ff (the code point '\uFB00', represented by the single code unit '\uFB00') come before '😀' (the code point '\u1F600', represented by the UTF16 two code units '\uD83D\uDE00')?

The simplest thing is probably something like

Sort attributes according to the lexicographic order of their [[Key]] attributes, treating the value of each such attribute as a sequence of UTF-16 code unit values.

[nicolo-ribaudo]

Thank you!

[ljharb]

Initial review:

• Is https://tc39.es/proposal-import-attributes/#note-HostLoadImportedModule-referrer-Realm-Record an addition? or is it just green because it's a note.
• HostGetSupportedImportAttributes should s/same contents/same contents in the same order.

[nicolo-ribaudo]

Is https://tc39.es/proposal-import-attributes/#note-HostLoadImportedModule-referrer-Realm-Record an addition? or is it just green because it's a note.

It's green just because it's a note. Except for that one, all the other notes are new.

EDIT: I pushed 2b161db to mark new non-editorial notes.

HostGetSupportedImportAttributes should s/same contents/same contents in the same order.

That list is effectively used as a Set (it's only used at step 2.a of https://tc39.es/proposal-import-attributes/#sec-AllImportAttributesSupported), so I don't think that the stricter requirement would have any effect.

[ljharb]

true, that's a fair point. however, it means that it's idempotent, whereas currently it's not.

[nicolo-ribaudo]

I merged #133, the spec text is now ready for review!

[ljharb]

• In EvaluateImportCall step 11.e.ii, you get the keys, and then get and validate the values. Is there a reason to prefer that over getting the entries, and then validating the values? The observable difference ofc would be whether all the Gets were done before validation, or whether the result of validation determined how many Gets were done. It seems nice to me to unconditionally get every property, and then validate them.
• A minor bikeshed: can AttributesEqual be named ImportAttributesEqual just to ensure no confusion?
• why are we supporting with {}? i assume for consistency with import {} and export {}andconst {} = obj` etc?