Closed nicolo-ribaudo closed 1 year ago
EvaluateImportCall
EnumerableOwnPropertyNames
anywhere, did you mean EnumerableOwnProperties
?AllImportAttributesSupported
Record { [[Key]], [[Value]] }
, or use "For each ImportAttribute Record…"Static Semantics: ModuleRequests
ModuleRequest and ImportAttribute Records
([[Specificer]], [[Attributes]])
paris), but we don't define its equality when using "same", only when using AttributesEqual
.HostLoadImportedModule
AttributesEqual
If
statement to do an early return if [[Key]]
matches but [[Value]]
doesn't.Static Semantics: WithClauseToAttributes
StringValue
isn't defined on AttributeKey: StringLiteral
.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.
"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.
Thank you!
Initial review:
s/same contents/same contents in the same order
.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 2b161db4d6e8d77eba0c535488dfddc1abd6ba6d 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.
true, that's a fair point. however, it means that it's idempotent, whereas currently it's not.
I merged https://github.com/tc39/proposal-import-attributes/pull/133, the spec text is now ready for review!
AttributesEqual
be named ImportAttributesEqual
just to ensure no confusion?with {}
? i assume for consistency with import {}
and export {}and
const {} = obj` etc?In HostLoadImportedModule, Note 2: is this really something we can't specify? Have we documented some example cases that indicate that more than one of these three approaches might be appropriate?
Thanks @ljharb for the review, sorry it took this long to tackle it :) I opened https://github.com/tc39/proposal-import-attributes/pull/141.
- HostGetSupportedImportAttributes should
s/same contents/same contents in the same order
.
✔️
- 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.
✔️ Done! I agree with making the behavior as much internally consistent as possible. I tried thinking about other parts in the spec where we might be doing something similar, but this is probably the first time we validate a "dictionary" object with unknown keys.
- A minor bikeshed: can
AttributesEqual
be namedImportAttributesEqual
just to ensure no confusion?
✔️
- why are we supporting
with {}
? i assume for consistency withimport {}
and export {}and
const {} = obj` etc?
Yes, there has been some discussion in https://github.com/tc39/proposal-import-attributes/issues/94 (and you already wrote there that you didn't particularly like it :P)
tl;dr:
export {}
/ import {}
(const {}
is actually different, since it asserts that the value is not nullish)import json from "./mod" with {
// TODO: This is temporarily a JS file
// type: "json"
};
In HostLoadImportedModule, Note 2: is this really something we can't specify? Have we documented some example cases that indicate that more than one of these three approaches might be appropriate?
The previous "import assertions" spec in practice mandated "reject" (even if it happened when loading and not when parsing). The spec as is written right now does "clone", since "reject" ended up not being integratable in HTML. Unfortunately it's not really possible to mandate "clone but not reject/race", since once they are different entries in the imports cache they have separate behaviors. Also, one import passing with one attribute doesn't imply that also another import will pass with a different attribute.
Tbh I would just not include that note in ECMA-262 — it was originally there because the author wanted to express "we are specifying assertions, but a future proposal might introduce something else that has a different behavior". Now that the proposal has been changed, that note doesn't have it's original utility.
@tc39/ecma262-editors Do you prefer if I open a PR to ecma262 to help reviewing this?
@nicolo-ribaudo If that's not too burdensome for you, yes, that is my preferred way to review proposals.
I opened https://github.com/tc39/ecma262/pull/3057 to facilitate reviewing this proposal as an actual diff on top of ECMA-262.
Thanks everyone for the reviews!
This proposal reached conditional consensus for Stage 3 during the March 2023 TC39 meeting.
The consensus is conditional on https://github.com/tc39/proposal-import-attributes/pull/133 being merged, and on Stage 3 reviews. This issue tracks the review status.