Fix: support ecmaFeatures.jsx flag

[bradzacher]

Fixes #594

[jsf-clabot]

All committers have signed the CLA.

[platinumazure]

See comment from armano2. Thanks for contributing!

[kaicataldo]

I'm not a big fan of having two ways to do the same thing. Any chance we can decide on one way to do this and be consistent about it (deprecating the old way if we decide on parserOptions.ecmaFeatures.jsx)?

My vote would be to make it consistent with the default parser and make parserOptions.ecmaFeatures.jsx the canonical way to set this and to deprecate parserOptions.jsx. babel-eslint does this with ecmaFeatures.globalReturn as well.

[armano2]

@kaicataldo we should support only one way to configure it, but we can't do breaking changes without major release.

typescript-estree expect jsx field, but its not in same way as eslint specifies it

plugins are setting this property as parserOptions.jsx,

[platinumazure]

We'll be doing a major release with all the syntax changes in the typescript-estree upgrades @armano2, don't worry about that. Next release will probably be major. 😄

[armano2]

this could be done as hotfix -> and it will fix issues for users using configs and plugins like eslint-plugin-react within this version :>

[bradzacher]

So what was the goal for this?
Would you like me to refactor to remove options.jsx, or is this good as is?

I'm happy to raise a second PR so that we can release both a patch and a breaking fix?

[armano2]

i think we should merge this first, do patch release, and wait for "eslint team" to decide how they are going to handle releases for new version of estree

[JamesHenry]

@bradzacher I'm back to reviewing things on this side of the fence again :)

I will be doing a major release of the library as soon as we merge #596, so let's go for what we've deemed the most correct solution here.

[bradzacher]

I have dropped support for inputting options.jsx.
It will now only respect options.ecmaFeatures.jsx.

[JamesHenry]

Sorry for the conflicts, @armano2 is your "approved" review status still valid on this?

[armano2]

there is only this pointed out by @kaicataldo and i'm unsure what to do with this...

[kaicataldo]

Sorry, I didn’t realize that this was already the current behavior. I’ll resolve my comment!

[platinumazure]

At this point, I think the only thing that would need to change is the commit summary (as this is technically a breaking change), but we could handle that on merge.

@kaicataldo @JamesHenry If either of you decides to merge this, feel free to dismiss this review and change the commit summary to start with "Breaking:".

[JamesHenry]

If it's ok with @bradzacher (he's on the core team for the new typescript-eslint org) I would rather get typescript-eslint/typescript-eslint#6 over the line and press on from there.

We can quickly put this and the other PRs together on that repo

[bradzacher]

Happy with whatever is easier. This is a simple pr to remake on the new repo :)

[kaicataldo]

Closing this PR since the project has been moved to the TypeScript ESLint organization. Feel free to reopen the PR there.