-
Notifications
You must be signed in to change notification settings - Fork 26
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
Normative optional, legacy -> Deprecated #133
Conversation
114955c
to
28e5f6d
Compare
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.
i like it!
cc @tc39/ecma262-editors and @syg in particular |
28e5f6d
to
df6ae55
Compare
spec.emu
Outdated
@@ -121,7 +124,7 @@ | |||
1. Return _promiseCapability_.[[Promise]]. | |||
1. Let _assertionsObj_ be Completion(Get(_options_, *"with"*)). | |||
1. IfAbruptRejectPromise(_assertionsObj_, _promiseCapability_). | |||
1. [normative-optional-legacy=""] If _assertionsObj_ is *undefined*, then | |||
1. [deprecated=""] If _assertionsObj_ is *undefined*, then |
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.
This isn't a "deprecated subclause". We should probably just have a NOTE step here that gets replaced in a deprecated subclause, much like what we do in Annex B. Search for "replaces-step" for examples to copy.
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.
I was following the path introduced by tc39/ecma262#2952, which inlines Annex B changes to the algorithms rather than replacing NOTE steps out-of-band.
I could update the description of "deprecated" to be "deprecated subclause, step, or production", or I could update this step to be
1. If the host supports the deprecated assert keyword for import attributes and if _assertionsObj_ is *undefined*, then
with "assert keyword for import attributes" linking to an actual deprecated emu-clause that explains what it is.
What do you think about these two options? I would also be happy to join an editors call if needed.
(I pushed a commit to separate the grammar into its own clause, since that ecma262 PR doesn't inline the grammar changes)
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.
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.
@nicolo-ribaudo We'll discuss it during this week's editor call. Feel free to join.
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.
Wording for dfn of deprecated LGTM, but we need to work on the mechanics of actually applying it, as I've described in my comments.
757f896
to
30ab06f
Compare
51ea854
to
d15915f
Compare
It'd be great to land this so i can review a spec that includes it :-) |
@ljharb We're gonna talk about it in editor call tomorrow. |
2f59bdd
to
e1a16a2
Compare
@tc39/ecma262-editors I pushed e1a16a2 based on yesterday's discussion. Preview of this PR: https://nicolo-ribaudo.github.io/proposal-import-assertions |
spec.emu
Outdated
<li>It is a Syntax Error if AllImportAttributesSupported(WithClauseToAttributes of |AssertClause|) is *false*.</li> | ||
</ul> | ||
|
||
<p>The static semantics of WithClauseToAttributes in <emu-xref href="#sec-with-clause-to-attributes"></emu-xref> are augmented with the following:</p> |
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.
I don't think it's unclear that we're inserting these (as opposed to replacing the existing ones), but I think I'd prefer to be more explicit with wording like
<p>The static semantics of WithClauseToAttributes in <emu-xref href="#sec-with-clause-to-attributes"></emu-xref> are augmented with the following:</p> | |
<p>The static semantics of WithClauseToAttributes in <emu-xref href="#sec-with-clause-to-attributes"></emu-xref> are augmented to include the following:</p> |
Also, since we are disallowing these AssertClause
nodes from being parsed in the first place using early errors, we don't technically need these definitions to be conditional. We could just put them in WithClauseToAttributes
. @tc39/ecma262-editors thoughts?
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.
I think it's nice to keep the deprecated feature as much self contained as possible, rather than scattered across different parts of the spec. If a spec reader doesn't care about it, it's much easier to skip when reading.
Just published ecmarkup 16.2.0, which supports |
Co-authored-by: Jordan Harband <[email protected]> Co-authored-by: Michael Ficarra <[email protected]>
0410d0a
to
be30172
Compare
d922219
to
94172e2
Compare
Thanks, updated! |
@tc39/ecma262-editors I plan to merge this PR during the weekend, if there aren't other problems with the changes it introduces. I'd like to get the published spec text (https://tc39.es/proposal-import-attributes/) to its final form to facilitate reviews (#137). |
PR preview: https://nicolo-ribaudo.github.io/proposal-import-assertions
As per the March 2023 TC39 meeting, this pull request changes the
assert
compatibility fallback from "Normative optional, legacy" to "Deprecated", with this description:(old description I first used)
For comparison, the "Legacy" description is
Additionally, I added two notes mentioning that removal of
assert
is being investigated.cc @ljharb @msaboff