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

Use @babel/core#parse #711

Merged
merged 22 commits into from Jan 10, 2019

Conversation

@kaicataldo
Copy link
Member

kaicataldo commented Nov 7, 2018

refs #573

This PR updates babel-eslint to require @babel/core as a peer dependency and to use the user's Babel configuration when parsing files for ESLint. Up until now, babel-eslint has manually enabled all plugins (with the list falling out of date frequently). It also meant that babel-eslint could parse syntax that a configured instance of Babel itself didn't allow at compile time.

This change makes it so that the same Babel configuration is used for the parser both for linting and for compilation, and should eliminate a whole class of bugs as well as give the end user more fine-grained control over what the parser can and cannot parse.

Note that this is a breaking change, as using babel-eslint will now require @babel/core@>=7.2.0 and a valid Babel configuration file to run (see #711 (comment) for some discussion around this).

@kaicataldo kaicataldo referenced this pull request Nov 7, 2018

Closed

Use @babel/core#parse #594

Show resolved Hide resolved lib/parse.js Outdated

@kaicataldo kaicataldo force-pushed the kaicataldo:babel-parse-take-2 branch 2 times, most recently from 969f3d9 to 8ac09b7 Nov 7, 2018

Show resolved Hide resolved README.md Outdated
Show resolved Hide resolved README.md Outdated

@kaicataldo kaicataldo force-pushed the kaicataldo:babel-parse-take-2 branch 2 times, most recently from 149e643 to 038f828 Nov 7, 2018

@kaicataldo kaicataldo force-pushed the kaicataldo:babel-parse-take-2 branch from 038f828 to 133f838 Nov 12, 2018

@kaicataldo

This comment has been minimized.

Copy link
Member

kaicataldo commented Nov 29, 2018

For anyone coming across this - I'm just waiting for babel/babel#8986 to be merged and released to finish this up.

@sibelius

This comment has been minimized.

Copy link

sibelius commented Dec 6, 2018

@kaicataldo it is merged 💯

@tomwidmer

This comment has been minimized.

Copy link

tomwidmer commented Dec 6, 2018

@kaicataldo

This comment has been minimized.

Copy link
Member

kaicataldo commented Dec 12, 2018

Planning to pick this up again this week. Here's an interesting edge case that I don't think was considered with this change: eslint/eslint#11189 (comment). Thoughts?

@ljharb

This comment has been minimized.

Copy link

ljharb commented Dec 12, 2018

I don’t think it’s reasonable to be concerned with a flagged, experimental feature in node that is still highly subject to a lot of change; nor do i think babel-eslint needs to be concerned with non-babel-users.

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Dec 17, 2018

I agree with @ljharb here 👍

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Dec 17, 2018

I can't tell from the PR. Is there a way to configure the location of the babel configuration as one of the parserOptions or something? With a toolkit (where the configuration does not necessarily live at the root of the project) being able to specify configuration directly would be really helpful.

@kaicataldo kaicataldo force-pushed the kaicataldo:babel-parse-take-2 branch 3 times, most recently from 382e29c to 7a97f15 Dec 18, 2018

@kaicataldo

This comment has been minimized.

Copy link
Member

kaicataldo commented Dec 18, 2018

@kentcdodds This was discussed elsewhere (trying to find it and am failing - might've been on Slack 😅) and I hadn't gotten around to it yet. Added here. This was also useful for testing!

Show resolved Hide resolved README.md Outdated

@kaicataldo kaicataldo force-pushed the kaicataldo:babel-parse-take-2 branch from c99c3ae to d781b53 Dec 18, 2018

@kaicataldo

This comment has been minimized.

Copy link
Member

kaicataldo commented Dec 18, 2018

This is ready for review/discussion. I'd love to figure out how to make these tests easier to manage, but I think the implementation here is reasonable and we can iterate in the future. Let me know how you feel.

Would also love to get the team's feelings about throwing when the required version of @babel/core isn't found. Also, am I missing anything configuration-wise? I know a lot changed in babel@7 and I'm still not super familiar with all the options surrounding configs, particularly in monorepos.

Edit: This actually isn't quite working, but I'd still love some eyes on the general approach here to see what people think. Thanks!

Edit: Looks like it's working now. I was missing passing through the filename that's now required since babel.config.js allows for overrides.

@kaicataldo kaicataldo force-pushed the kaicataldo:babel-parse-take-2 branch from dd5cd20 to a342401 Dec 18, 2018

@hzoo

hzoo approved these changes Jan 9, 2019

@kentcdodds
Copy link
Member

kentcdodds left a comment

This is wonderful! Thank you!

globalReturn: false,
},
babelOptions: {
configFile: "path/to/config.js",

This comment has been minimized.

@kentcdodds

kentcdodds Jan 9, 2019

Member

This is fantastic. Very important for toolkits that encapsulate eslint config. Thank you for adding this feature 💯

@ljharb

ljharb approved these changes Jan 9, 2019

Copy link

ljharb left a comment

this will unblock the airbnb eslint config from using babel-eslint, which also unblocks it from using stage 3 features. the internet will be very pleased with this, as am i :-)

@SimenB SimenB referenced this pull request Jan 9, 2019

Open

Version 1.0.0 #151

@cKiril

This comment has been minimized.

Copy link

cKiril commented Jan 9, 2019

Hey all, when can we expect this to roll out to production?

@hzoo hzoo referenced this pull request Jan 9, 2019

Closed

Official ESLint 5.x support? #664

@kaicataldo

This comment has been minimized.

Copy link
Member

kaicataldo commented Jan 10, 2019

I've been continuing to do some manual testing, and everything seems to be working correctly. I have two things that I wanted to get feedback on (but I don't think they need to be blockers for landing this):

  1. With the default ESLint results formatter, the Parsing error: babel-eslint@11.0.0 does not support @babel/core@7.1.6. Please downgrade to babel-eslint@^10 or upgrade to @babel/core@>=7.2.0 error message will currently be printed out for each file linted. We could just print it once in the beginning, but I think it's fine to leave it as is because we can't lint the file anyway and need to show the user some kind of error.
  2. It looks like babelCore#parseSync will still parse without a config file, it just won't enable any plugins. Does this seem like a reasonable default? I'm worried about it being confusing behavior since Babel itself won't run any transformations without a config file. An alternative would be to run a check to see if a config file can be found (not sure if @babel-core provides an API for this already or not) and throw an error with a helpful message, if not.

Again, I don't think either of these are critical to landing this PR, but I'd love to make sure we decide what we want to do with these cases before we actually make a release. Please feel free to merge this if you all think it's ready to go. cc @loganfsmyth Wouldn't mind your review if you have some time!

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jan 10, 2019

An alternative would be to run a check to see if a config file can be found (not sure if @babel-core provides an API for this already or not) and throw an error with a helpful message, if not.

I am in favor of this. If babel-eslint cannot find the config, then it's likely someone misconfigured something. Maybe just a warning log would be ok though? And an option to disable the warning could be helpful as well.

@kaicataldo

This comment has been minimized.

Copy link
Member

kaicataldo commented Jan 10, 2019

@kentcdodds How does it sound for me to merge this as-is and then for me to make some issues for the two questions I have above so we can discuss?

@existentialism

This comment has been minimized.

Copy link
Member

existentialism commented Jan 10, 2019

@kaicataldo I'm down with that

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jan 10, 2019

I'm good with that. This would definitely be a nice-to-have 👍

@SimenB

This comment has been minimized.

Copy link

SimenB commented Jan 10, 2019

You can do loadOptions and throw/warn if you don't find any config? If found, pass it as second param to parse.

https://babeljs.io/docs/en/babel-core#loadoptions

@loganfsmyth

This comment has been minimized.

Copy link
Member

loganfsmyth commented Jan 10, 2019

I agree about filing it separately. The issue is that not all files in your project may have a config. If you have a .babelrc in src/ but didn't bother with any Babel config for the rest of your project, you should still be able to lint those files using the parser's defaults.

@kaicataldo

This comment has been minimized.

Copy link
Member

kaicataldo commented Jan 10, 2019

Great! Will make some issues for us to discuss. Thanks for the input, everyone - it's been super helpful 👍

@kaicataldo kaicataldo merged commit a861223 into babel:master Jan 10, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kaicataldo kaicataldo deleted the kaicataldo:babel-parse-take-2 branch Jan 10, 2019

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jan 10, 2019

celebretory gif

Woo! Thanks!

@kaicataldo

This comment has been minimized.

Copy link
Member

kaicataldo commented Jan 10, 2019

Issue to discuss how to handle missing config files here: #738

I'm not going to make an issue for my first question because I now think the current behavior is the best way to go. Feel free to comment if you disagree though.

@arahansen

This comment has been minimized.

Copy link

arahansen commented Jan 14, 2019

The README changes added here implies this change has already been released (or that is me optimistically reading into the description). Are these changes published anywhere?

@kaicataldo

This comment has been minimized.

Copy link
Member

kaicataldo commented Jan 14, 2019

It hasn't been released yet. We just decided on a path forward for this last issue today and, pending landing that, we should be able to get a release out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment