Skip to content
This repository has been archived by the owner on Aug 18, 2021. It is now read-only.

Add support for customizing parser plugins #692

Closed
wants to merge 6 commits into from
Closed

Add support for customizing parser plugins #692

wants to merge 6 commits into from

Conversation

g-plane
Copy link

@g-plane g-plane commented Sep 26, 2018

Maybe partially fix #505 .

This PR enables a feature that user can enable or disable babel parser plugins via parserOptions in .eslintrc file. This can let us parse TypeScript code with babel.

If anyone can't wait this PR, you can try: https://github.com/g-plane/pluggable-babel-eslint

@g-plane g-plane mentioned this pull request Sep 26, 2018
@sibelius
Copy link

can we use typescript plugin when the file has extensions .ts or .tsx
and use flow plugin when the file has extensions .js or .jsx

using the extensions approach you don't even need to add typescript as a parser option

we are migrating incrementally to typescript using this approach https://medium.com/entria/incremental-migration-to-typescript-on-a-flowtype-codebase-515f6490d92d

@nstepien
Copy link

nstepien commented Sep 26, 2018

@sibelius You can use overrides: https://eslint.org/docs/user-guide/configuring#disabling-rules-only-for-a-group-of-files
For example in my config file I currently have:

  overrides: [{
    files: ['*.ts', '*.tsx'],
    parser: 'typescript-eslint-parser',
    parserOptions: {
      sourceType: 'module'
    },
    rules: tsRules
  }, {
    files: ['*.tsx'],
    parserOptions: {
      ecmaFeatures: {
        jsx: true
      }
    }
  }, ...]

@g-plane
Copy link
Author

g-plane commented Sep 26, 2018

@sibelius babel-eslint is just a parser and it isn't able to detect what the extension is.

@kaicataldo
Copy link
Member

Just so you're aware, another approach to solve this problem was discussed here. The PR that I started (and unfortunately was unable to land at the time) is here.

I personally think that reading the Babel config is the right approach, because you then don't have to manage and keep in sync what is essentially two separate Babel configs.

I'm hoping to get back to my PR sometime, but I just haven't had the time unfortunately.

@g-plane
Copy link
Author

g-plane commented Sep 27, 2018

So can this PR keep open?

@nstepien
Copy link

@hzoo @existentialism
Can we get this looked into? I'd love to use babel-eslint for ts files.

@sibelius
Copy link

@g-plane can you read babel config from file and use it in this PR?

@g-plane
Copy link
Author

g-plane commented Sep 28, 2018

@sibelius Yeah I will try but not sure whether I can do it well or not.

@g-plane
Copy link
Author

g-plane commented Sep 29, 2018

It's OK to support .babelrc, but it seems that it isn't easy to support babel.config.js.

@g-plane
Copy link
Author

g-plane commented Sep 30, 2018

I don't add the feature that supports reading Babel config now, but I have made a file which may resolve the Babel config. Everyone can review that file first and comment here. Link: https://github.com/g-plane/pluggable-babel-eslint/blob/master/lib/config/project-wide.js

If the logic of that file has no problems, I will add it to babel-eslint itself later.

@kaicataldo
Copy link
Member

@g-plane The original implementation that I was working on let babel itself do all the configuration resolution, which I thought was a good thing because it meant that the config resolution logic wouldn't have to be duplicated (easier to maintain, smaller surface area for bugs, etc.).

My work obviously fell by the wayside, and I'm unsure of how the Babel v7 config file changes affect the implementation I was working on. Do you feel like this shouldn't happen in Babel core? Thanks for looking at this!

@g-plane
Copy link
Author

g-plane commented Oct 4, 2018

I don't think using "@babel/core" directly is a good idea, because it has some dependencies like "@babel/generator", which is unnecessary. However, I'm not opposed to create a new npm package that uses "@babel/core".

@skyrpex
Copy link
Contributor

skyrpex commented Oct 4, 2018

Is having unnecessary dependencies the only con about using @babel/core? I never take that very seriously since it doesn't affect in any way other than the total node_module size after install...

@hzoo
Copy link
Member

hzoo commented Oct 4, 2018

because it has some dependencies like "@babel/generator", which is unnecessary

The reason why I would think this isn't an issue is because we made an assumption that if you are using babel-eslint you are using Babel, and thus already would have @babel/core and everything else as a dependency (we would make it a peerDep)

@g-plane
Copy link
Author

g-plane commented Oct 4, 2018

Hmm...well, if @kaicataldo 's PR is merged, I will close this PR.

@kaicataldo
Copy link
Member

@g-plane I'm taking some time off over the next month and will be able to work on this again. That being said, if someone beats me to it, they should feel free!

@kaicataldo
Copy link
Member

#711 has landed

@hzoo
Copy link
Member

hzoo commented Jan 10, 2019

Closing since ^ landed!

@hzoo hzoo closed this Jan 10, 2019
@hzoo
Copy link
Member

hzoo commented Jan 10, 2019

Although if we want to support TS here, we may need some of the other code in this PR?

@kaicataldo
Copy link
Member

Yeah, that's something that maybe should be coordinated with typescript-estree.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typescript Support
6 participants