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

add support for preset & plugins in babelOptions #784

Closed
wants to merge 1 commit into from

Conversation

jharris4
Copy link

As discussed in a comment at the end of #711 I have a use case for the following:

parserOptions: {
  requireConfigFile: false,
  babelOptions: {
    presets: [ /* presets go here */ ],
    plugins: [ /* plugins go here */ ]
  }
}

The change to the code in this PR was trivial and I confirmed locally that it does work as expected, allowing me to pass in a custom babel config without using a babel config file.

Writing the test case took a couple of hours as I had to figure out a way to show that the change does exactly what is expected.

Feedback on improving/cleaning-up the tests would be more than welcome...

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

This looks good to me 👍

@jharris4
Copy link
Author

jharris4 commented Aug 1, 2019

@kentcdodds Thanks for reviewing and approving!

Are two approvals needed for a PR to be merged? I don't see anything in CONTRIBUTING.md.

Would love to be able to use this change once it's merged and released to a new beta... ;-)

@kentcdodds
Copy link
Member

I'm not really a maintainer. I don't know what the process for things like this are in this repository. So I'll wait for a maintainer to review this.

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.

One concern I have with this change is that it feels really arbitrary what config options babel-eslint would be allowing and not allowing.

I believe both TypeScript and typescript-eslint require a tsconfig.json, and this feels like a really similar case to me.

That being said, I'm not part of the core team, and they might disagree with me! Would be interested to see what they have to say.

[
{
matcher: message => message.startsWith("1:25 Parsing error:"),
message: "1:25 Parsing error: <path> <rest of message>",
Copy link
Member

Choose a reason for hiding this comment

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

This error message is a bit confusing to me - is that what's coming from Babel's parser?

Copy link
Author

Choose a reason for hiding this comment

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

yes, it's the error from Babel.

To show that the new feature is working, I had to devise a test case showing that the babel configuration gets changed by the provided config.

Unfortunately babel's error messages contain the full path to the file (which is machine dependent) so I couldn't just match the strings exactly.

And in fact, showing that the correct lint error is shown in one case (with correct babel config) and that babel errors (and the exact error text doesn't matter) in the other case seemed like an adequate test.

I'm all ears if someone has suggestions for a better way to test the feature though...

undefined,
{
parserOptions: {
requireConfigFile: false,
Copy link
Member

Choose a reason for hiding this comment

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

Is it expected that this option would always be used in conjunction with requireConfigFile: false? I'm curious what the behavior would be when this is true.

We should also probably update the README if we're going to make this change.

Copy link
Author

Choose a reason for hiding this comment

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

if you're not using a babel config file then you need to disable requireConfigFile. Seemed clear to me, so my thought is that the existing documentation is good enough. ;-)

@jharris4
Copy link
Author

One concern I have with this change is that it feels really arbitrary what config options babel-eslint would be allowing and not allowing.

A babel config is a babel config regardless of where it comes from. The only thing changing here is where this config gets specified, but the available settings/options are exactly the same.

I believe both TypeScript and typescript-eslint require a tsconfig.json, and this feels like a really similar case to me.

A lot of tools do follow the convention of requiring a configuration file to be named a certain way or for it to be in a specific location. There's nothing wrong with that per se, but it is short sited to force this to be the case (in my opinion) since it limits interoperability with other tools.

Better to support multiple options and let users or build tool authors pick the approach that works best for them... ;-)

@axten
Copy link

axten commented Sep 5, 2019

@hzoo @kaicataldo Is it possible to merge this and release that as another v11 beta? this way we could test this strongly needed feature... Thanks

@jharris4
Copy link
Author

jharris4 commented Sep 5, 2019

@axten, yes it'd be great to get this merged, and then I could tackle making another PR for 11.x to fix the eslint 6.3 issues...

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

I think that babel-eslint should accept Babel config as a programmatic option, because config loading should be consistent accross tools: babel-loader, rollup-plugin-babel and @babel/core all allow specifying the config without using an external file.

That said, I think that the correct approach would be to pass down to Babel an object containing all the provided options without hard-coding them. The options object should be opaque to babel-eslint.

I think that we should do something like this:

if (options.babelOptions.parserOpts) console.warn("Ignored");
if (options.babelOpts.caller) console.warn("ignored");

let opts = {
  sourceType: options.sourceType,
  filename: options.filePath,
  ...options.babelOptions,
  
  parserOpts: {
    allowImportExportEverywhere: options.allowImportExportEverywhere,
    allowReturnOutsideFunction: true,
    allowSuperOutsideMethod: true,
    ranges: true,
    tokens: true,
    plugins: ["estree"]
  },
  caller: {
    name: "babel-eslint"
  }
}

@axten
Copy link

axten commented Dec 13, 2019

anything new here? we're still unable to use private class methods because of this not been merged...

@nicolo-ribaudo
Copy link
Member

@axten Private methods are broken for another reason. If you are already using private methods with Babel, then babel-eslint should already be able to parse them.

@axten
Copy link

axten commented Dec 17, 2019

We don't have a babel config file, so we're unable to use any newer babel plugin since months.
Private class methods are not working for us until #801 is released in the v10.x branch or #768 lands in the v11.x branch.

@jharris4
Copy link
Author

@axten it's not ideal, but since there hasn't been a new release of this package in quite some time we've been using a fork with this PR included.

It's based off the 11.x branch, and you can find it on npm as babel-eslint-fork.

We've been using it for a few months now without any issues.

@axten
Copy link

axten commented Dec 17, 2019

thank you, I will have a look...

@hadnet
Copy link

hadnet commented Dec 22, 2019

@axten it's not ideal, but since there hasn't been a new release of this package in quite some time we've been using a fork with this PR included.

It's based off the 11.x branch, and you can find it on npm as babel-eslint-fork.

We've been using it for a few months now without any issues.

I'm using it but I got the same error: "This experimental syntax requires enabling the parser plugin: 'classPrivateMethods"

@jharris4
Copy link
Author

@hadnet I haven’t tried classPrivateMethods, but it sounds like you maybe need to tweak your babel config?

If you share your config and a sample of code using private class methods I can take a look...

@hadnet
Copy link

hadnet commented Dec 22, 2019

@hadnet I haven’t tried classPrivateMethods, but it sounds like you maybe need to tweak your babel config?

If you share your config and a sample of code using private class methods I can take a look...

const developmentEnvironments = ['development', 'test'];

const developmentPlugins = [require('react-hot-loader/babel')];

const productionPlugins = [
  require('babel-plugin-dev-expression'),

  // babel-preset-react-optimize
  require('@babel/plugin-transform-react-constant-elements'),
  require('@babel/plugin-transform-react-inline-elements'),
  require('babel-plugin-transform-react-remove-prop-types')
];

module.exports = api => {
  // see docs about api at https://babeljs.io/docs/en/config-files#apicache

  const development = api.env(developmentEnvironments);

  return {
    presets: [
      [
        require('@babel/preset-env'),
        {
          targets: { electron: require('electron/package.json').version }
        }
      ],
      require('@babel/preset-flow'),
      [require('@babel/preset-react'), { development }]
    ],
    plugins: [
      // Stage 0
      require('@babel/plugin-proposal-function-bind'),

      // Stage 1
      require('@babel/plugin-proposal-export-default-from'),
      require('@babel/plugin-proposal-logical-assignment-operators'),
      [require('@babel/plugin-proposal-optional-chaining'), { loose: false }],
      [
        require('@babel/plugin-proposal-pipeline-operator'),
        { proposal: 'minimal' }
      ],
      [
        require('@babel/plugin-proposal-nullish-coalescing-operator'),
        { loose: false }
      ],
      require('@babel/plugin-proposal-do-expressions'),

      // Stage 2
      [require('@babel/plugin-proposal-decorators'), { legacy: true }],
      require('@babel/plugin-proposal-function-sent'),
      require('@babel/plugin-proposal-export-namespace-from'),
      require('@babel/plugin-proposal-numeric-separator'),
      require('@babel/plugin-proposal-throw-expressions'),

      // Stage 3
      require('@babel/plugin-syntax-dynamic-import'),
      require('@babel/plugin-syntax-import-meta'),
      [require('@babel/plugin-proposal-private-methods'), { loose: true }],
      [require('@babel/plugin-proposal-class-properties'), { loose: true }],
      require('@babel/plugin-proposal-json-strings'),

      ...(development ? developmentPlugins : productionPlugins)
    ]
  };
};

When I set my eslintrc to 'parser': 'babel-eslint-fork', I got another error: "Parsing error: params is not iterable."

When I'm using the arrow function way no error at all.

@jharris4
Copy link
Author

@hadnet I don’t see anything obviously wrong, and I see there’s a related issue: #728

I’m traveling for the holidays but will give it a try when I’m back at my computer.

In the meantime, have you tried switching the order of the private-methods and class-properties plugins so that class properties is first?

@jharris4
Copy link
Author

@hadnet I finally had some time to look into using the @babel/plugin-proposal-private-methods, and it's installing/activated correctly using this plugin, but I get an error when linting:

0:0 error Parsing error: params is not iterable

It looks like this is a known issue #749, and it seems that it's blocked until a PR gets merged into estree...

@axten
Copy link

axten commented Mar 12, 2020

@hzoo @kaicataldo @nicolo-ribaudo this project seems to be unmaintained. I cannot understand why this don't get merged and released. Several people are waiting for this since july...

@kaicataldo
Copy link
Member

Hey there, apologies for not getting back to you sooner. All active development has moved to the main Babel monorepo. You can track our progress here. Do you mind making an issue with your proposal in that repository so we can evaluate it in the context of our v8 release plans?

@kaicataldo
Copy link
Member

Closing this now that babel/babel#11639 has been released. Thanks for contributing!

@kaicataldo kaicataldo closed this Aug 4, 2020
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.

None yet

6 participants