Skip to content

Add tsdx lint command#99

Merged
jaredpalmer merged 34 commits into
jaredpalmer:masterfrom
skvale:feat/linting-command
Aug 14, 2019
Merged

Add tsdx lint command#99
jaredpalmer merged 34 commits into
jaredpalmer:masterfrom
skvale:feat/linting-command

Conversation

@skvale

@skvale skvale commented May 11, 2019

Copy link
Copy Markdown
Contributor
  • Add tsdx lint command
  • Use tsdx lint to lint itself

@skvale skvale mentioned this pull request May 11, 2019
Use it to lint itself
@jaredpalmer

Copy link
Copy Markdown
Owner

Sickkkkk

@jaredpalmer

Copy link
Copy Markdown
Owner

This looks great. Will give it try in the morning

Comment thread src/createEslintConfig.ts Outdated
Comment thread src/createEslintConfig.ts Outdated
@jaredpalmer

Copy link
Copy Markdown
Owner

@skvale I think these rules are a bit too tight. Can we just use eslint-config-react-app ?

@skvale

skvale commented May 30, 2019

Copy link
Copy Markdown
Contributor Author

@jaredpalmer I made createEslintConfig return

{
  extends: ['react-app'],
};

For tsdx, I ran yarn lint --write-file and updated the .eslintrc.js to

module.exports = {
  extends: [
    'react-app',
    'prettier/@typescript-eslint',
    'plugin:prettier/recommended',
  ],
};

We could document the ways of customizing the rules/extensions through

  • .eslintrc.js
  • packageJson.eslint

in the README

@jaredpalmer

Copy link
Copy Markdown
Owner

Since we install prettier for people, we should setup eslint to play nicely? Does eslint config react app do that?

@skvale

skvale commented May 30, 2019

Copy link
Copy Markdown
Contributor Author

Yep, I'm pretty sure prettier and eslint-config-react-app work well together.

How about adding a --prettier optional flag to lint, like yarn lint --prettier instead of having to generate a .eslintrc.js file or add the rules to the package.json? 4332881

@jaredpalmer

Copy link
Copy Markdown
Owner

Run it by default. The less flags the better

@jaredpalmer jaredpalmer changed the title feat(lint): Add tsdx lint command Add tsdx lint command May 31, 2019
@jameslnewell

jameslnewell commented Jun 11, 2019

Copy link
Copy Markdown

🎉Nice!

Can we add a --watch option?

What's the thinking behind providing a separate lint command vs running the linter on build or test? (my DX preference would be to discover and fix linting issues while I'm working in the file where the problems exist rather than having to come back after I've edited a bunch of files - similar to CRA).

@skvale

skvale commented Jun 22, 2019

Copy link
Copy Markdown
Contributor Author

@jaredpalmer

Copy link
Copy Markdown
Owner

Let's resolve conflicts and get this merged?

@skvale

skvale commented Jun 22, 2019

Copy link
Copy Markdown
Contributor Author

@jaredpalmer Sounds good
I changed the config to account for react/create-react-app@24489ac

and bumped to "@typescript-eslint/parser": "^1.10.3-alpha.13" for the issue from typescript-eslint/typescript-eslint#628

@swyxio swyxio mentioned this pull request Jul 24, 2019

@leohxj leohxj left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In vs code, eslint does't lint typescript file by default.Do we need a vscode settings file to make eslint lint ts file?

Comment thread package.json Outdated
"@typescript-eslint/parser": "^1.12.0",
"ansi-escapes": "^3.2.0",
"asyncro": "^3.0.0",
"babel-eslint": "^10.0.2",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

does this necessary?
I think use the @typescript-eslint/parser parser is enough.

@skvale

skvale commented Jul 27, 2019

Copy link
Copy Markdown
Contributor Author

@leohxj I had to do two things to get eslint to work in vscode. This page for create-react-app explains it well too: https://facebook.github.io/create-react-app/docs/setting-up-your-editor

  • tsdx lint --write-file <-- generate the local config file
  • Add typescript to my settings file in eslint.validate
  "eslint.validate": [
    "javascript",
    "javascriptreact",
    "typescript",
    "typescriptreact"
  ],

@jaredpalmer

Copy link
Copy Markdown
Owner

Is this g2g?

@skvale

skvale commented Aug 14, 2019

Copy link
Copy Markdown
Contributor Author

yeah, I think so

@jaredpalmer

Copy link
Copy Markdown
Owner

Not for this PR, but we may want to think through some kind of upgrade and/or validation command. This would check package.json, dotfile config, tsconfig, eslint etc. and fill in (and/or reset) any missing pieces. IIRC, jest has something like this called like jest-validate?.

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

Successfully merging this pull request may close these issues.

6 participants