create-react-app: New Typescript linting overriding user config and causing build failure

Our ESLint configuration is not currently configurable (as has always been the case) and is documented here: https://facebook.github.io/create-react-app/docs/setting-up-your-editor#displaying-lint-output-in-the-editor. Any changes you make to your configuration file will only affect your editor integration but not the Dev server or prod builds.

We haven’t ever supported TSLint so I’m not sure how the other comments are related.

@ianschmitz I think the confusion was that Typescript folks where using TSLint since the react-scripts had no TS linting support. We have entire projects built using custom rules, but now since TS linting is supported in v3, we are all stuck with pages of warnings and/or broken builds. While the JS community had somewhat the ease of greenfield development using react-scripts’ blessed linting rules, the TS community is now coming onboard with custom opinionated rulesets for large existing projects.

Proposals:

1. provide a way to completely disable linting in production and live server (maybe this already exists?)
2. provide a way for custom eslint overriding rules (both JS/TS)

On a community aside, I didn’t realize the react-scripts didn’t allow JS projects to customize the linting rules. As someone that disagrees with the defaults of most linters (as well as my team), it feels very heavy-handed to including linting by-default without a way to customize. (obviously there’s ejection, but it’s far from an easy path to own)

Hi. I’m just chiming in here because I got side-swiped by having to upgrade create-react-app from version 2.1 to 3.0 this weekend, for a project that is fairly mature, in order to fix my build times for my react app.

Please correct me if I’m wrong, but my understanding is:

• my upgrade moves me away from react-scripts-ts to react-scripts
• I can have an .eslintrc.json file to help my IDE/local development environment
• I can’t disable react build script warnings because the team thinks that’s inappropriate, and react-scripts provides no ability to do so
• Which means all of my builds will have failures because my development settings don’t align with the build settings
• There is no clear work around

If this is is the case, I am beyond disappointed in what the perceived user experience is for this.

A developer is always going to want to toggle these settings. All of these settings may or may not directly apply to specific projects. And now the product create-react-app is blasting noise every time I build which makes it impossible to discern what is a failure, and what is actually something I need to address. Which means that pushes all of that output into my IDE. And even worse, is these are displayed as warnings in a lot of instances when they should be failures blocking me until I fix them.

Folks, this is terrible, and I would like to articulate that this lack of support is a big disappointment that makes me very keen to move away from create-react-app as it basically removes a significant amount of functionality that made it attractive in the first place.

@ztolley The baked in linting configuration is how we’ve always provided CRA, including adding new rules between major versions which is similar to what is being mentioned in this thread. This thread is slightly different in that some (including myself) were using TSLint prior to 3.0. To clarify, we did mention that the added TypeScript linting was in the 3.0 release notes and listed as a breaking change.

I will discuss the possibility of extending the ESLint config with the team which i think would satisfy what most of you are looking for (i know i would use it personally), but i can’t promise anything as it’s a fairly significant change to how we’ve been providing CRA for a few years now.

@ianschmitz in my case, the linting is not necessarily breaking, but confusing in a few ways:

1. VSCode picks up my local .eslintrc.js file, so VSCode provides linting warnings and autofixes from that file. My linting tasks from package.json also use my local .eslintrc.js file. CRA, however, uses its own config and reports separate warnings in the terminal and browser console. It’s confusing for my project to have two essentially two different linting rule sets, and there are certain rules that come with CRA that I just don’t want (see below), or aren’t included in the default config.

2. When creating a production build, my builds fail since process.env.CI = true and warnings are treated as errors. I’m aware I can disable this.

@gaearon rules I’m bumping into:

no-unused-vars - I have unused vars all the time when I comment out code during development, so while I like the idea of this rule, I generally find it a hindrance while developing and turn it off. What I used to do with TSLint was occasionally turn the rule on, run my linter to clean up unused code, and then turn it back off.

array-callback-return - I’m getting this from an arrow function that does a switch case on a known TypeScript string, so I have no default return value. default-case - Same as above. Example:

          titles.sort((titleA, titleB) => {
            switch (titleSortBy) {
              case 'name': {
                return getAlphabeticalSortValue(titleA.name, titleB.name);
              }
              case 'type': {
                return getAlphabeticalSortValue(titleA.type, titleB.type)
              }
              case 'launchDate': {
                return getNumericSortValue(titleA.launchDate, titleB.launchDate);
              }
            }
          });

It looks like @ianschmitz already addressed this, though I’m not sure that PR also covers array-callback-return ?

exhaustive-deps - I’m new to hooks here so maybe I’m doing something wrong, but it seems like you shouldn’t need to pass in certain types of functions, like dispatch or action creators like fetchTitle:

  // only run this effect when props.movieId changes
  useEffect(() => {
    props.dispatch(fetchTitle(props.movieId));
  }, [props.movieId]);

The example here makes sense since doSomething is closure containing someProp, but in this case I’m not sure the linting rule makes sense?

Even if I adopted all the rules above, there are additional rules I like that aren’t in the CRA configuration, such as import/order –– it looks like the default CRA config has no opinion about import ordering, for instance.

@ianschmitz I understand there may be a compelling philosophical reason to not allow linting customization in CRA, but is there a compelling code complexity reason? It seems like loading a user’s custom .estlinrc file would be relatively straightforward (per my comment above), but I’m not nearly familiar enough with the codebase to make that judgment. Would it have consequences on other parts of the CRA build pipeline?

@samueldepooter: I’d file another issue along the lines of “eslint lints compilation output instead of the source code”.

Seems like it’s a general eslint / integration problem: it lints compilation artifacts in various runtimes instead of checking the source code.

Not pretending to know how others use lint, for me it’s a tool to enforce a set of rules about how the source should be written and structured (which I call a style guide), that the-project (whatever the entity it represents) came up with. The coming-up-with seems to be a very important point, since if the set of rules is fixed, some kind of bare-minimum-to-prevent-bugs, it no longer serves the goal of enforcing the style guide, instead serving somebody else’s goal of not allowing me to make mistakes.

I find such enforcing useful as a pre-commit / pre-build check, not as a continuous runtime thing. In a continuous dev environment I, for example, don’t care that a variable I just declared is “never used” or that a certain method I just added is not in the right position. Besides, the instant notification can be obtained by number of other means, such as analysis provided by the editor of choice and its plugins, when/if wanted (with a very fine control of what and when it is wanted).

So like the author of this issue summarized above, there seem to be a misunderstanding going on about the value of lint between CRA maintainers and its users, where the maintainer seek to protect users from bugs, and the users want to be able to lint their code.

So the outcome of this is:

• the existing users of tslint are puzzled by the unnecessary step they see little to no value in that CRA3 added
• the existing users of tslint who are ready to move to eslint can not do so, because (even if eslint and tslint were 100% compatible right now) they are unable to configure eslint within cra3
• the existing users of eslint are puzzled by the fact that their linting is being taken away
• users that were not using any kind of linting are puzzled by the fact that the cra3 linting causes build failures, being plugged into everything automatically instead of being made available as a separate opt-into step

cra3 effectively takes linting away from projects who lint, replacing it with a safety feature.

opt-out exists on the file level only, that is via /* eslint-disable */.

that is my current understanding of the problem

3.1 comes with support for custom eslint configuration, should that solve most of the problems here?

https://github.com/facebook/create-react-app/pull/7036

Having the ability to define/override rules in eslintrc.json is pretty important…

@ianschmitz I know there’s a lot of noise, but if you have a moment, I tried to address your questions and Dan’s above in https://github.com/facebook/create-react-app/issues/6871#issuecomment-487682863.

You mentioned in a followup comment that you are considering the override functionality – is this something that’s being tracked in an issue or RFC or is it still in a conceptual phase?

TSLint is getting deprecated, as its own homepage says:

I’m not sure what longer term strategy you’re suggesting if even TypeScript itself is officially recommending a move to ESLint (which we have done). Create React App 3.0 aligns the linting rule with the same rules that have always been enabled in the JavaScript version.

If there are specific rules that are causing you trouble, I’d appreciate a more detailed writeup about specifics. There is no need to make this issue emotional. We’re all people and are trying to do what’s good for the project and most users. I’m sorry upgrading created issues for your existing setups — but eventually you’d have to make those changes anyway, as TSLint will be deprecated by its maintainers.

If you want to totally disable the build warning overlays, you can add this css to your project as a short term / quick fix:

iframe[style="position: fixed; top: 0px; left: 0px; width: 100%; height: 100%; border: none; z-index: 2147483647;"] {
        display: none;
}

We have started a PR to extend ESLint config: #7036.

@bradzacher I see what you’re saying now. Thanks for clarifying. I’m not sure what the best path forwards is. Perhaps with #7036, users could disable that rule if it’s causing issues for them. They could enable noUnusedLocals as a replacement, but it would be active during development currently which is a little unfortunate.

@estaub sadly it is the entire contents… I found an older cra issue that is comparable to what you’re saying: https://github.com/facebook/create-react-app/issues/2772

FYI my app runs fine, it’s only in storybook. And the error also doesn’t output in the story file itself unless I force it by actually violating the rule. So it must be something after compilation…

EDIT: I finally figured it out. My storybook setup had an addon plugged into webpack which had an enforce: 'pre'. It adds extra code on compilation which made ESLint fail on the rule import/first

It seems like I’m having the same issue with CRA 3 & Storybook 5…

import React from 'react';
import { storiesOf } from '@storybook/react';
const stories = storiesOf('button', module);

stories.add('button', () => <button>click me</button>);

will throw

WARNING in Button/Button.stories.tsx
Module Error (from ./node_modules/eslint-loader/index.js):

Line 6:3:  Import in body of module; reorder to top  import/first

To my knowledge I’m not violating any rules?

I upgraded from CRA 2 with tslint to CRA 3 with default eslint

// .eslintrc
{
  "extends": ["react-app"]
}

Ok I found a workaround for this issue: Use react-app-rewired with customize-cra (https://github.com/arackaf/customize-cra) and something like the following config:

// config-overrides.js
const {
    override,
    useEslintRc,
} = require('customize-cra');

module.exports = override(
    useEslintRc(),
);

And use something like this as .eslintrc.js

// .eslintrc.js
'use strict';

const baseConfig = require('eslint-config-react-app');
const baseOverrides = Array.isArray(baseConfig.overrides) ? baseConfig.overrides : [baseConfig.overrides];
const baseTsOverride = baseOverrides.find(x => x.files.find(f => f.indexOf('*.ts') > 0));

module.exports = {
    ...baseConfig,
    overrides: [
        {
            ...baseTsOverride,
            rules: {
                ...baseTsOverride.rules,

                // Remove with next npm release of eslint-config-react-app:
                'default-case': 'off',
                'no-useless-constructor': 'off',
                '@typescript-eslint/no-useless-constructor': 'warn',
                'no-dupe-class-members': 'off',

                // << add your own custom rules here >>
            },
        }
    ],
    rules: {
        ...baseConfig.rules,
        // << add your own custom rules here >>
    },
};

I’m glad I was already using react-app-rewired so this was actually quite easy to add \o/ But it would be really nice if you could just define custom rules for those 2 sections directly through create-react-app instead of doing some hacky stuff like this. It would be so easy to allow users to define something like customEslintRules and customEslintTsRules which could be merged into those places

We’ve also upgraded to the latest react-scripts (3.0.0) in order to get rid of high vulnerabilities, but it broke our tslint configuration. No .eslinttignore or .eslintrc are working and adding comments to every file that disables eslint seems really bad.

AFAIK namespaces are still not supported.

Edit: Ah, so there is some experimental support … https://babeljs.io/docs/en/babel-plugin-transform-typescript/

I like to structure my TS code using namespaces to group functions, for example:

export type Option<A> = Some<A> | None<A>

export namespace Option {
  export const none = <A>(): Option<A> => new None<A>()
  export const some = <A>(a: A): Option<A> => new Some<A>(a)
}

...

CRA seems to want to tell me how to write my code:

Compiling...
Failed to compile.

./src/util/Option.ts
  Line 3:  ES2015 module syntax is preferred over custom TypeScript modules and namespaces  @typescript-eslint/no-namespace and namespaces  @typescript-eslint/no-namespace

This is a purely stylistic point - giving a compilation error! - and actually one that makes zero sense, since namespaces are actually useful.

How do I get rid of this useless error?

(the workaround, for those others frustrated with this limitation, is to declare a const object with a property for each namespace member)

I’m using simply a glob pattern, ./src/**/*.{ts,tsx} plus maxwarning option equal to 0 to prevent commits when there are any warnings (by default warnings on CI are treated as errors).

@mbaranovski You need to do this … https://facebook.github.io/create-react-app/docs/setting-up-your-editor#displaying-lint-output-in-the-editor

@zwhitchcox https://github.com/arackaf/customize-cra#disableeslint is probably the best option right now.

@ianschmitz - sorry, I meant that our rule (@typescript-eslint/no-unused-vars) isn’t 100% correct. Typescript has some very complicated scope logic, so there are a number of false positives.

What I was meaning is that our no-unused-vars is great for development because it can be non-build-blocking, but it’s not great as a production build blocker because of the false positives.

The best experience is to use our no-unused-vars in dev only, and noUnusedLocals for the prod build only.

Taking us way off topic here (sorry everyone 😃, buuuuut…

they’ll be forced to handle it in the switch statement at compile time.

That only occurs if you strictly type the function return and/or use noImplicitReturns.

This can then be handled by instead using the following helper, which will trigger a type error if someone forgets to add a case to the switch because TS won’t do an implicit never cast:

function assertIsNever(value: never) {
  return new Error(`Unexpected value ${JSON.stringify(value)}`);
}

/////

default:
  // Argument of type '"b"' is not assignable to parameter of type 'never'.
  throw assertIsNever(value);

playground

I would imagine more than a handful of organisations have CRA-based projects with tslint.json file referring to their own common ruleset as "extends": ["@company/tslint-config-company"].

CRA 3 breaks any such project by enforcing a different set of rules and it’s a shame to eject because of it.

While moving towards ESLint and providing some reasonable defaults for linting are both very reasonable goals, I think the existence of local tslint or eslint configuration files should definitely override this.

It’s likely most developers aren’t even aware of TSLint’s future deprecation plans yet, so this is likely a very unpleasant surprise while updating projects dependencies.

Perhaps a more reasonable migration path would be to display a deprecation warning about TSLint configuration, so users can start migrating their rules.

@ianschmitz Thanks for your reply and your patience. In my case I can get around it for now. I feel for the people who can’t and I think an opt out would help them and be a quick win.

I also agree that a measured approach to addressing this and providing further flexibility is the right thing to do, rather than rush into a solution that makes things worse with regards to picking up a users config.

Ok, but why can’t I configure my own rules? To comply with your rules, I would have to rewrite my code. Stupid breaking change… I also came back to revert to 2.

I’m having the same issue: tslint.json rules seem to be ignored now in v3 and overwritten by the new defaults. Please send help.

I had the same issue but I found a solution: https://graphql-code-generator.com/docs/plugins/add

In your codegen.yml, use the plugin add to insert an /* eslint-disable */ line to the start of the file.

Example:

schema: http://localhost:5000/api/graphql
overwrite: true
generates:
    ./src/generated.tsx:
        documents: ./src/graphql/*.ts
        plugins:
            - add: "/* eslint-disable */"
            - typescript-common
            - typescript-client
            - typescript-react-apollo
        config:
            noNamespaces: true
            enumsAsTypes: true
            withHooks: true
            noHoc: false
            noComponents: false