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

Stage 3 syntax review #94

Closed
waldemarhorwat opened this issue Sep 14, 2020 · 8 comments · Fixed by #95
Closed

Stage 3 syntax review #94

waldemarhorwat opened this issue Sep 14, 2020 · 8 comments · Fixed by #95

Comments

@waldemarhorwat
Copy link

The syntax seems fine. I have one question: Why can't the list of AssertEntries be empty?

@waldemarhorwat
Copy link
Author

Sorry, this should be stage 3 review not 2.

@waldemarhorwat waldemarhorwat changed the title Stage 2 syntax review Stage 3 syntax review Sep 14, 2020
@ljharb
Copy link
Member

ljharb commented Sep 14, 2020

why have assertion syntax if you aren't asserting anything?

there is export {} and const {} = obj, but both seem pretty confusing to have allowed :-/

@michaelficarra
Copy link
Member

@ljharb Maybe you've commented them all out.

@dandclark
Copy link
Collaborator

dandclark commented Sep 15, 2020

Normally I'd be skeptical about allowing empty AssertEntries given the potential for confusion and the minor extra complexity with minimal benefit. In other contexts people probably wouldn't expect an empty assert statement/function to be permitted.

But since we allow import {} from "./foo.js", export {}, etc I think it might make sense to allow an empty AssertEntries for consistency's sake:

import { } from "./foo.js" assert { }; // No error. Would be potentially surprising if the first
                                       // set of empty brackets was allowed but not the second.

@dandclark
Copy link
Collaborator

If we want to go this route, I created #95. Other proposal champions, @ljharb, any strong opposition to this? I'm lightly in favor for the consistency reason stated in my previous comment.

dandclark added a commit that referenced this issue Sep 15, 2020
@waldemarhorwat pointed out during Stage 3 review that the current syntax does not permit an empty AssertClause.  Though an empty AssertClause would not be semantically different from leaving out the AssertClause altogether, it seems reasonable to allow an empty one for consistency's sake because other parts of `import`/`export` syntax permit empty lists.  For example it would be strange if the first `{ }` was permitted in the following statement (which is currently the case), but the second was not:
`import { } from "./foo.js" assert { }`.

Thus this PR edits the spec to allow an AssertClause that has no entries.

Closes #94.
@MylesBorins
Copy link
Member

I'm going to reopen this for now as I don't think we've gotten a +1 for all review. @dandclark please reclose if that was a mistake

@MylesBorins MylesBorins reopened this Sep 15, 2020
@waldemarhorwat
Copy link
Author

The revised syntax looks good.

@littledan
Copy link
Member

The proposal reached stage 3, so re-closing

@nicolo-ribaudo nicolo-ribaudo mentioned this issue May 3, 2023
3 tasks
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 a pull request may close this issue.

6 participants