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

feat: Support ES2023 and hashbangs #556

Merged
merged 5 commits into from
Aug 24, 2022
Merged

feat: Support ES2023 and hashbangs #556

merged 5 commits into from
Aug 24, 2022

Conversation

btmills
Copy link
Member

@btmills btmills commented Aug 21, 2022

Acorn enabled the hashbang grammar when ecmaVersion is 2023 in acornjs/acorn@b2ecf7a, which was released in v8.8.0.

This change adds 14/2023 as valid ecmaVersions (which automatically increments latest).

When Acorn's constructor encounters #! at the beginning of input, it transitively calls our onComment() handler. I therefore needed to move Espree's state to before the super() call and make it usable before this is accessible.

Copy link
Member

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

@btmills btmills requested a review from aladdin-add August 22, 2022 03:09
Copy link
Member

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

👍

@mdjermanovic
Copy link
Member

Hashbangs will have type: "Line", like // comments. Should we maybe introduce a new comment type, e.g. "Shebang" (to match how ESLint works)?

@nzakas
Copy link
Member

nzakas commented Aug 22, 2022

I’m ambivalent about the token name. On one hand, ESLint itself handles hashbangs already, and we can’t remove that functionality for backwards compatibility, so it really doesn’t matter what the token is.

The question is more what would consumers other than ESLint expect this token to be?

By default, the `ecmaVersion` test runner was not asking the parser for
comments. Now, when the source filename contains `comment`, it sets
`comment: true` and includes comments in the expected result.

This extends the precedent set by filenames containing strings like
`"non-strict"`, `"edge-cases"`, `"modules"`, and a few others, all of
which put the test runner in slightly different modes.
Reviewers asked for the hashbang comment to have a type distinct from
`"Line"` [1].

ESLint already handles hashbangs internally, and removing its custom
handling would break backwards compatibility. The name is therefore up
to whatever non-ESLint users of Espree would expect. [2]

The hashbang grammar proposal already addressed this choice directly
[3], so this commit defines the type as `"Hashbang"` to match the
proposal.

[1]: #556 (comment)
[2]: #556 (comment)
[3]: https://github.com/tc39/proposal-hashbang#why-hashbang-instead-of-shebang
@btmills
Copy link
Member Author

btmills commented Aug 23, 2022

The proposal includes a section on "why hashbang instead of shebang". Since ESLint's precedent of calling it "Shebang" is less relevant here, I went with "Hashbang" to be consistent with the proposal, which is likely what non-ESLint consumers would expect.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants