Skip to content

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

Merged
nzakas merged 13 commits intomainfrom
class-fields
Jun 11, 2021
Merged

Update: add class fields (refs eslint/eslint#14343)#486
nzakas merged 13 commits intomainfrom
class-fields

Conversation

@mysticatea
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

nzakas commented Apr 28, 2021

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

@nzakas
Copy link
Copy Markdown
Member

nzakas commented Apr 29, 2021

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

mysticatea and others added 4 commits May 7, 2021 11:58
This reverts commit 3e9caa7.
Co-authored-by: 薛定谔的猫 <weiran.zsd@outlook.com>
@nzakas
Copy link
Copy Markdown
Member

nzakas commented May 20, 2021

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown

Any news?

@nzakas
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

There are merge conflicts that should be resolved

@nzakas
Copy link
Copy Markdown
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
Copy Markdown

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