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

Update: add class fields (refs eslint/eslint#14343) #486

Merged
merged 13 commits into from
Jun 11, 2021
Merged

Conversation

mysticatea
Copy link
Member

@mysticatea mysticatea commented Apr 28, 2021

WIP.

This PR adds ES2022 class features.


Remaining steps:

  1. Revert 3e9caa7 to remove dist.
  2. Release eslint-visitor-keys.
  3. Update the eslint-visitor-keys version range in package.json.

Also, because I expect ESLint built-in rules have false positives about the new syntax, I hope to release this feature together with built-in rules updates.

@mysticatea
Copy link
Member Author

Hmmm. The build step makes developing new syntax on cross-packages difficult. 😟

npm install github:eslint/espree#class-fields doesn't work because dist doesn't exist. Are any alternative ways?

@aladdin-add
Copy link
Member

for local development, you can use npm link

However, I don't think there is an easy way to run on CI.

@mdjermanovic
Copy link
Member

npm install github:eslint/espree#class-fields doesn't work because dist doesn't exist. Are any alternative ways?

Maybe comment out dist in .gitignore, build locally and push dist/ to this branch (we should revert that before merging).

@nzakas
Copy link
Member

nzakas commented Apr 28, 2021

Switched to a draft for work-in-progress changes.

@nzakas
Copy link
Member

nzakas commented Apr 29, 2021

You can also use a preinstall npm script. That might be easier to remove before merging.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
tools/update-ecma-version-tests.js Outdated Show resolved Hide resolved
mysticatea and others added 4 commits May 7, 2021 11:58
This reverts commit 3e9caa7.
Co-authored-by: 薛定谔的猫 <weiran.zsd@outlook.com>
package.json Outdated Show resolved Hide resolved
@nzakas
Copy link
Member

nzakas commented May 20, 2021

@mysticatea what is left to do on this PR? Is anything we can help with?

package.json Outdated Show resolved Hide resolved
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!

@aladdin-add
Copy link
Member

@mysticatea seems it is ready to go (the deps has been released & updated).

@nzakas nzakas marked this pull request as ready for review June 3, 2021 20:31
@sanex3339
Copy link

Any news?

@nzakas
Copy link
Member

nzakas commented Jun 7, 2021

@sanex3339 all news is posted on the pull request.

Comment on lines +11 to +17
sourceType: "module"
},
overrides: [
{
files: ["tests/lib/**"],
files: ["*.cjs"],
parserOptions: {
ecmaVersion: 2020
},
sourceType: "script"
Copy link
Member

Choose a reason for hiding this comment

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

@mysticatea doesn't plugin:node/recommended already handle this?

Comment on lines +319 to +320
"type": "PrivateIdentifier",
"value": "a",
Copy link
Member

Choose a reason for hiding this comment

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

Token's value intentionally doesn't contain #?

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

Given that the work appears done and we haven’t heard back from @mysticatea, I think we should merge this and handle any small issues separately.

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!

@mdjermanovic
Copy link
Member

There are merge conflicts that should be resolved

@nzakas
Copy link
Member

nzakas commented Jun 11, 2021

Yup, all fixed now.

@nzakas nzakas merged commit e71162c into main Jun 11, 2021
@nzakas nzakas deleted the class-fields branch June 11, 2021 18:20
@sanex3339
Copy link

Thank you for this PR

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.

5 participants