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

Add tsdx lint command #99

Merged
merged 34 commits into from Aug 14, 2019
Merged

Add tsdx lint command #99

merged 34 commits into from Aug 14, 2019

Conversation

skvale
Copy link
Contributor

@skvale skvale commented May 11, 2019

  • 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
Owner

Sickkkkk

@jaredpalmer
Copy link
Owner

This looks great. Will give it try in the morning

src/createEslintConfig.ts Outdated Show resolved Hide resolved
src/createEslintConfig.ts Outdated Show resolved Hide resolved
@jaredpalmer
Copy link
Owner

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

@skvale
Copy link
Contributor Author

skvale commented May 30, 2019

@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
Owner

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

@skvale
Copy link
Contributor Author

skvale commented May 30, 2019

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
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
Copy link

jameslnewell commented Jun 11, 2019

🎉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
Copy link
Contributor Author

skvale commented Jun 22, 2019

@jaredpalmer
Copy link
Owner

Let's resolve conflicts and get this merged?

@skvale
Copy link
Contributor Author

skvale commented Jun 22, 2019

@jaredpalmer Sounds good
I changed the config to account for facebook/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
Copy link

@leohxj leohxj left a comment

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?

package.json Outdated
"ansi-escapes": "^3.2.0",
"asyncro": "^3.0.0",
"babel-eslint": "^10.0.2",
Copy link

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
Copy link
Contributor Author

skvale commented Jul 27, 2019

@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
Owner

Is this g2g?

@skvale
Copy link
Contributor Author

skvale commented Aug 14, 2019

yeah, I think so

@jaredpalmer
Copy link
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.

None yet

6 participants