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

Docs: Add new experimental syntax policy to README (fixes #9804) #10408

Merged
merged 3 commits into from
Jun 9, 2018

Conversation

platinumazure
Copy link
Member

See #9804 (comment).

What is the purpose of this pull request? (put an "X" next to item)

[x] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain: Applying a policy update, so may need to be discussed at TSC meeting.

What changes did you make? (Give an overview)

Updated README to clarify experimental language feature policy. Specifically, we accept PRs to avoid crashes (only) for Stage 3 issues; we otherwise require Stage 4 (as we have before now) for rule features and enhancements; and we treat language extensions on a case-by-case basis. (We could add more info about those language extensions in a later update, I think.)

Is there anything you'd like reviewers to focus on?

I used the language drafted by @nzakas in issue #9804, but we should review it carefully since this is basically a policy change. (Might need TSC discussion too, since we didn't have unanimous 👍 from all TSC on that issue, although we had no 👎 either.)

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label May 28, 2018
@platinumazure platinumazure added documentation Relates to ESLint's documentation core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion tsc agenda This issue will be discussed by ESLint's TSC at the next meeting and removed triage An ESLint team member will look at this issue soon labels May 28, 2018
@platinumazure
Copy link
Member Author

TSC Summary:

This pull requests attempts to clarify our README with the experimental feature policy suggested by Nicholas here: #9804 (comment)

TSC Question:

Does this PR capture the intent of the new policy accurately and clearly? Should we make this change? (Less importantly, are there other documentation areas that should be similarly changed?)

@ljharb
Copy link
Sponsor Contributor

ljharb commented May 29, 2018

The only thing that might not be captured here is, what about fixing a core rule that crashes with a non-default parser? Hopefully that's included in the intention of this change.

@platinumazure
Copy link
Member Author

platinumazure commented May 29, 2018

The only thing that might not be captured here is, what about fixing a core rule that crashes with a non-default parser? Hopefully that's included in the intention of this change.

Honestly, I'm not sure whether it is or isn't. We should probably change the language either way. I'll make sure we discuss it in the TSC meeting.

My own take would be it should be case-by-case-- there's no way we could promise to fix crashes for any parser out there, especially if the real problem ends up being that the parser is not actually conforming to ESTree.

Edit: I think it does, because Stage 3 features are (by our espree policy) not implemented in espree the default parser, so it would be any Stage 3 feature on a non-default parser. Other than Stage 3, popular language extensions are case by case (as stated) and Stage 0-2 proposals would be a no.

README.md Outdated
ESLint doesn't natively support experimental ECMAScript language features. You can use [babel-eslint](https://github.com/babel/babel-eslint) to use any option available in Babel.
ESLint's parser only officially supports the latest final ECMAScript standard. We will make changes to core rules in order to avoid crashes on stage 3 ECMAScript syntax proposals. We may make changes to core rules to better work with language extensions (such as JSX, Flow, and TypeScript) on a case-by-case basis.

In other cases (including if rules need to warn on more or fewer cases due to new syntax, rather than just not crashing), you should use other parsers and/or rule plugins. If you are using Babel, you can use the [babel-eslint](https://github.com/babel/babel-eslint) parser and [eslint-plugin-babel](https://github.com/babel/eslint-plugin-babel) to use any option available in Babel.
Copy link
Member

Choose a reason for hiding this comment

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

This is very nitpicky, but I prefer we recommend you use other parsers and/or rule plugins to you should use other parsers and/or rule plugins, because I think it's a little friendlier!

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, I'll make that change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I lied. Not sure I like that because this really is a (gentle) order to use something that isn't espree. How about please use other parsers and/or plugins? Or I can do your wording if you prefer.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer the wording I suggested, but I don't feel that strongly about it. Totally up to you :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't feel strongly about it, so I'll go with your wording. Expect a new commit shortly.

Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

Thanks for clarifying this!

Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

One tweak about specifying ESTree, but otherwise LGTM. I particularly like the helpful note about eslint-plugin-babel. Nice work!

README.md Outdated
@@ -146,7 +146,9 @@ ESLint has full support for ECMAScript 3, 5 (default), 2015, 2016, 2017, and 201

### What about experimental features?

ESLint doesn't natively support experimental ECMAScript language features. You can use [babel-eslint](https://github.com/babel/babel-eslint) to use any option available in Babel.
ESLint's parser only officially supports the latest final ECMAScript standard. We will make changes to core rules in order to avoid crashes on stage 3 ECMAScript syntax proposals. We may make changes to core rules to better work with language extensions (such as JSX, Flow, and TypeScript) on a case-by-case basis.
Copy link
Member

Choose a reason for hiding this comment

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

As discussed in the TSC meeting, can you specify that not crashing in core rules is limited to ESTree's experimental AST representations for stage 3 syntax?

Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

New wording LGTM

@platinumazure platinumazure added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion tsc agenda This issue will be discussed by ESLint's TSC at the next meeting labels Jun 9, 2018
@platinumazure
Copy link
Member Author

This was accepted in the 2018-06-07 TSC meeting.

@platinumazure platinumazure merged commit 3721841 into master Jun 9, 2018
@platinumazure platinumazure deleted the platinumazure-patch-1 branch June 9, 2018 14:07
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Dec 8, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features documentation Relates to ESLint's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants