Skip to content
This repository has been archived by the owner on Jan 26, 2019. It is now read-only.

Replace TSLint with eslint-config-react-app-ts #388

Closed
wants to merge 13 commits into from

Conversation

nickmccurdy
Copy link

@nickmccurdy nickmccurdy commented Sep 2, 2018

Continuation of #354 with merge conflicts resolved.

Fixes #333.

Depends on eslint-config-react-app-ts.

@wmonk
Copy link
Owner

wmonk commented Sep 2, 2018

I would like to carry on the mentality set out by CRA originally. As this package is currently recommended by them for Typescript support within CRA I would like to keep it this way.

So easy to get started with, but if you want extra control then you'll have to eject.

@wmonk
Copy link
Owner

wmonk commented Sep 2, 2018

@nickmccurdy regarding the linting rules as a separate package, can we bundle it with react-scripts-ts so that it can be extended by a nested path, like react-scripts-ts/eslint-config?

@nickmccurdy
Copy link
Author

nickmccurdy commented Sep 2, 2018

Unfortunately no, the package name has to start with eslint-config- (scopes are also supported).

https://eslint.org/docs/developer-guide/shareable-configs

We would need to publish a separate package to share the ESLint config.

@nickmccurdy
Copy link
Author

nickmccurdy commented Sep 3, 2018

I'm experimenting with extracting the config into a separate package. I'd appreciate some dogfooding on existing projects to determine how well it works outside CRA-TS.

eslint-config-react-app-ts

@nickmccurdy nickmccurdy changed the title Replace TSLint with CRA's ESLint config Replace TSLint with eslint-config-react-app-ts Sep 3, 2018
@slikts
Copy link

slikts commented Sep 4, 2018

How can I run this? I checked out the repo and the eslint branch, installed it, npm linked it, but running create-react-app --scripts-version react-scripts-ts still uses the non-eslint version.

@wmonk
Copy link
Owner

wmonk commented Sep 4, 2018

The app you just created will not use the linked version. If you now run npm link react-scripts-ts in app directory it should now work.

@slikts
Copy link

slikts commented Sep 4, 2018

I don't get how linking in the tslint app would help me create the eslint version.

@wmonk
Copy link
Owner

wmonk commented Sep 4, 2018

When you run create-react-app --scripts-version=react-scripts-ts it installs the package internally. Once installed it does not know about your linked one, furthermore running npm link does not affect all projects using that module.

These are the steps:

# in checkout out repo
npm link

# in place you want to create new project
npm create react-app test-eslint-ts --scripts-version=react-scripts-ts
cd test-eslint-ts
npm link react-scripts-ts

@slikts
Copy link

slikts commented Sep 4, 2018

I understand the steps, but I don't understand how just linking to the new react-scripts-ts would help me if the app is already created and is the wrong (tslint) one.

@wmonk
Copy link
Owner

wmonk commented Sep 4, 2018

Create react app works by calling commands on a sub package's binaries. Usually react-scripts, but in our case react-scripts-ts. If you look at your package json you can see start: react-scripts-ts start etc. This is pointing to the file somewhere like ./node_modules/.bin/react-scripts-ts, which is a symlink to ../node_modules/react-scripts-ts/bin or something like that. For that reason you have the tslint version in use, so now you need to link your project to your locally checked out eslint version.

@slikts
Copy link

slikts commented Sep 4, 2018

I get and did all of that, but what is the point? There's still tslint.json and it still uses tslint; I can't even run the project if tslint.json is removed.

@nickmccurdy
Copy link
Author

nickmccurdy commented Sep 4, 2018

It looks like it's still not using this linked branch.

I just tried this:

  1. npm link in latest clone of ESLint branch
  2. cd ~
  3. npx create-react-app --scripts-version react-scripts-ts example in home directory
  4. cd example
  5. rm tslint.json to confirm this will break unless we're on the right branch
  6. npm link react-scripts-ts
  7. npm start launches successfully with ESLint instead of TSLint

Can you post your generated app, show your output of npm ls, or answer if npm ls eslint-config-react-app-ts (which is only installed on this branch) has results?

I get and did all of that, but what is the point? There's still tslint.json and it still uses tslint; I can't even run the project if tslint.json is removed.

This is not what's supposed to happen and not what happens for me. What is supposed to happen in development is that tslint.json is still generated (because you can't use a linked package when generating with create-react-app, but this wouldn't be an issue for end users) but is ignored, which is why I can delete it and still run scripts normally.

@slikts
Copy link

slikts commented Sep 5, 2018

Sorry, you're right, it works; not sure what the issue previously was.

@wmonk
Copy link
Owner

wmonk commented Sep 29, 2018

I'd really like to get this in - having just done a lot of work on #409 on brand new projects i can only imagine what a pain the current linting setup is for people new to dev/ts/react so would like to alleviate that ASAP.

@nickmccurdy do you think we could get this in with #409. Obviously all the upstream changes include the code for eslint, so could get it in there? I am really stumped by the eslint extending naming convention, seems like an oversight for things like this package to export a config instead of publishing a new package. The only way I got it to work was by doing like ./node_modules/react-scripts-ts/eslint.config.json - do you think that is too unfriendly?

@r3nya
Copy link
Contributor

r3nya commented Sep 29, 2018

Looks really good but I have concern about it. :\

@nickmccurdy Why we need to install another npm package? You can generate eslint config with your changes and it will work w/o extra dev deps. ¯\_(ツ)_/¯

{
  extends: "eslint-config-react-app",
  overrides: [
    {
      files: ["*.ts", "*.tsx"],
      parser: "typescript-eslint-parser",
      rules: {
        "no-array-constructor": "off",
        "no-multi-str": "off",
        "no-restricted-globals": "off",
        "no-undef": "off",
        "no-unused-vars": "off"
      }
    }
  ]
};

@slikts
Copy link

slikts commented Oct 2, 2018

eslint doesn't seem to be that well supported for TS; it's hard to find other examples of usage, most of them are incomplete, and I've seen no guides for it. The parser doesn't support the latest TS version. Rules like no-undef are broken. I also had to configure vscode-eslint to make this work.

@nickmccurdy
Copy link
Author

You can use typescript-eslint-parser, though it has some sharp edges

@slikts
Copy link

slikts commented Oct 2, 2018

typescript-eslint-parser is what I meant by "the parser"; it doesn't support TS 3.1.

@nickmccurdy
Copy link
Author

nickmccurdy commented Oct 21, 2018

I'm closing this now that facebook/create-react-app#4837 is merged into CRA, which uses eslint-config-react-app instead of TSLint's defaults. I'll still try to support eslint-config-react-app-ts if you want to use it.

@brunolemos
Copy link

@nickmccurdy just fyi eslint wasn't enabled on ts files on cra, see discussion

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.

Consider relaxing lint rules
6 participants