search

eslint / typescript-eslint-parser

An ESLint custom parser which leverages TypeScript ESTree to allow for ESLint to lint TypeScript source code.
Other
915 stars 75 forks source link

Fix: support ecmaFeatures.jsx flag #595

Closed bradzacher closed 5 years ago

bradzacher commented 5 years ago

Fixes #594

jsf-clabot commented 5 years ago

CLA assistant check
All committers have signed the CLA.

kaicataldo commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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. :smile:

armano2 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 https://github.com/eslint/typescript-eslint-parser/pull/596, so let's go for what we've deemed the most correct solution here.

bradzacher commented 5 years ago

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

JamesHenry commented 5 years ago

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

armano2 commented 5 years ago

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

kaicataldo commented 5 years ago

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

JamesHenry commented 5 years ago

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

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

bradzacher commented 5 years ago

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

kaicataldo commented 5 years ago

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