Skip to content
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

Webpack encore requires "babel-eslint" even if not used #685

Closed
david-wilson-mcn opened this issue Jan 11, 2020 · 3 comments
Closed

Webpack encore requires "babel-eslint" even if not used #685

david-wilson-mcn opened this issue Jan 11, 2020 · 3 comments

Comments

@david-wilson-mcn
Copy link

david-wilson-mcn commented Jan 11, 2020

What i'm trying to use is encore, typescript, react, and linting typescript with using the '@typescript-eslint/parser' parser (since tslint is being deprecated). I got it all working, but my question is why do I still need the 'babel-eslint' package if i'm not using the babel eslint parser? Encore complains that I need it if I enable 'enableEslintLoader'. Here is the relevant parts of my webpack.config.js:

    .enableTypeScriptLoader()
    .enableForkedTypeScriptTypesChecking()
    .enableEslintLoader(options => {
        options.parser = '@typescript-eslint/parser'
    })
    .configureLoaderRule('eslint', loader => {
        loader.test = /\.(jsx?|tsx?)$/
    })
    .enableReactPreset()

Here is the relevant parts of my .eslintrc.js file:

    parser: "@typescript-eslint/parser",
    plugins: [
        "@typescript-eslint",
        "react"
    ],
    extends: [
        "eslint:recommended",
        "plugin:@typescript-eslint/eslint-recommended",
        "plugin:@typescript-eslint/recommended",
        "plugin:react/recommended"
    ],
    parserOptions: {
        ecmaVersion: 2018,
        sourceType: 'module',
        ecmaFeatures: {
            jsx: true
        }
    }

If I don't have the package and run 'yarn dev-server' I get:

Running webpack-dev-server ...

  WARNING   Be careful when using Encore.configureLoaderRule(), this is a low-level method that can potentially break Encore and Webpack when not used carefully.
  Error: Install eslint-loader & babel-eslint to use enableEslintLoader()
    yarn add eslint-loader@^2.1.2 babel-eslint@^10.0.1 --dev

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command
@Kocal
Copy link
Contributor

Kocal commented Jan 11, 2020

Hi,

This is because Encore already configure ESLint a bit by setting parser to babel-eslint and so babel-eslint is required when using enableEslintLoader.

We already had an issue with because of this babel-eslint already configured (#574) and I've opened a RFC (#657) to make Encore stop configuring ESLint and let the end-user have a full control on ESLint configuration.

For now you have to keep package babel-eslint installed even if you don't use it. 😕

weaverryan added a commit that referenced this issue Mar 20, 2020
This PR was merged into the master branch.

Discussion
----------

Remove ESLint user-related config

Hi ✋

This PR is a proposal for #657 implementation and should fix issues encountered in #473, #574, #656, #659, and #685.

As discussed in #657, it would be better if Encore didn't configure ESLint itself. It should be done by the end user in an configuration file (e.g.: `.eslintrc.js`)

ESLint's `parser` option is not configured by default anymore, and `babel-eslint` requirement has been removed. We can considering this as a breaking change, end users should now configure `parser: 'babel-eslint` themselves.

Method `Encore.enableEslintLoader('extends-name')` has been deprecated and undocumented, in order to prevent end users to configure ESLint through Encore.

A nice message is also displayed when no ESLint configuration is found:
![Capture d’écran de 2020-01-12 11-15-09](https://user-images.githubusercontent.com/2103975/72217430-dfa2a480-352d-11ea-96e3-0e77236127d6.png)
I couldn't use `FriendlyErrorsPlugin` for this because error is not coming from Webpack. If someone has a better idea... 😕

**Pros:**
- end users have now full control hover ESLint configuration **by default**
- no need to `delete options.parser` when trying to use [`eslint-plugin-vue`](https://github.com/vuejs/eslint-plugin-vue) or [`@typescript-eslint/parser`](https://github.com/typescript-eslint/typescript-eslint/tree/master/packages/parser)
- IDEs will be able to integrate ESLint (if they support it)

**Cons:**
- end users should now configure `parser` option to `babel-eslint` themselves
- end users will have to move their config to external config file, but it's ok

What do you think?
Thanks.

**EDIT:** tests failures are unrelated I think.

Commits
-------

9d3b02f tweaking wording and order
d23982a Display message if no ESLint configuration is found
c828b32 Deprecate `Encore.enableEslintLoader('extends-name')`
9f31d95 Add test for ESLint with external configuration (and babel-eslint usage)
3ba6cf2 Remove babel-eslint from ESLint configuration
@Kocal
Copy link
Contributor

Kocal commented Mar 21, 2020

Hi, #687 has been merged.
It means that you will be able to uninstall babel-eslint at the next Encore release! :)

@weaverryan
Copy link
Member

Awesome :). Closing!

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

No branches or pull requests

3 participants