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

feat(gatsby-plugin-typescript): Supports linting #18721

Merged
merged 32 commits into from
Mar 2, 2020

Conversation

ThewBear
Copy link
Contributor

Description

Splitted from #16168 (comment)

eslint-config-react-app has supported linting Typescript since version 3.0.0 (facebook/create-react-app#6513).

It would be great if Gatsby also supports this.

Related Issues

#15976
#16168

@ThewBear ThewBear requested a review from a team as a code owner October 16, 2019 09:13
@ThewBear ThewBear changed the title feat(gatsby): Supports linting Typescript feat(gatsby): Supports linting Typescript through gatsby-plugin-typescript Oct 16, 2019
@pieh
Copy link
Contributor

pieh commented Oct 16, 2019

Hey @ThewBear this is looking great. Please merge master branch in (failing node 12 test was a problem some time ago in master and was fixed there, and it's not related to your code change)

@ThewBear ThewBear changed the title feat(gatsby): Supports linting Typescript through gatsby-plugin-typescript feat(gatsby-plugin-typescript): Supports linting Oct 16, 2019
@pieh
Copy link
Contributor

pieh commented Oct 16, 2019

In core we do check if user has their own eslint config before adding eslint rule (which is opinionated, so this is a way for users to disable builtin one if they have their own config) -

if (!hasLocalEslint(program.directory)) {
configRules = configRules.concat([rules.eslint(schema)])
}

And addition to that we only apply this loader in gatsby develop when building browser bundle.

In current implementation in this PR this part is missing, and it's not obvious how we could re-implement it (as hasLocalEslint function is internal one and not public one), but maybe we could examine config that gatsby build so far, and see if there is rule for eslint-loader to determine if typescript plugin should add config as well? This should cover both reservations?

Some quick code that could be clean up to check it:

const hasBuiltInEslintRule = webpackConfig => {
  try {
    return webpackConfig.module.rules.some(rule => {
      if (rule.enforce === `pre`) {
        return rule.use.some(use => {
          return /eslint-loader/.test(use.loader)
        })
      }
      return false
    })
  } catch {
    return false
  }
}

exports.onCreateWebpackConfig = ({ stage, getConfig }) => {
  console.log(`stage ${stage}: ${hasBuiltInEslintRule(getConfig())}`)
}

will output:

  • in gatsby develop if there is no eslint config in root of directory
    stage develop-html: false
    stage develop: true
    
  • in gatsby develop if there is eslint config in root of directory
    stage develop-html: false
    stage develop: false
    
  • in gatsby build (regardless of existance of eslint config file):
    stage build-javascript: false
    stage build: true
    

So this is something you could use to determine wether to add eslint rule for .tsx? files or not

@pieh
Copy link
Contributor

pieh commented Oct 25, 2019

All right, I was testing it and few notes:

We do need to add typescript as peerDependency to gatsby-plugin-typescript, update plugin readme so users can copy install command to install it too and in code do some conditional check if typescript is installed before adding the rule (potentially warn users that typescript is not installed and builtin eslint won't be working on typescript files).

Pretty confusingly, you can use gatsby-plugin-typescript without typescript being installed right now (as the way it's implemented it's using babel plugin and not actual typescript to compile the code).

@pieh pieh added the status: awaiting author response Additional information has been requested from the author label Oct 28, 2019
@LekoArts
Copy link
Contributor

@ThewBear Hey 👋 Do you have time to address the requested changes?

@LekoArts
Copy link
Contributor

@ThewBear Hey 👋 Kind reminder of the above ping 😊

@ThewBear
Copy link
Contributor Author

@LekoArts Will address this weekend.

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Nice one! This is great for typescript users. Could you perhaps add typescript as an optional dependency.

I've added 2 comments about variable names but besides that this is 👌

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Tested and works! :) Thanks for adding this one and for your patience!

@wardpeet wardpeet merged commit d28a0bd into gatsbyjs:master Mar 2, 2020
@wardpeet
Copy link
Contributor

wardpeet commented Mar 2, 2020

Fixed in [email protected]

@ThewBear ThewBear deleted the ThewBear-eslint-typescript branch March 2, 2020 09:50
@pvdz
Copy link
Contributor

pvdz commented Mar 3, 2020

Looks like in some cases (at least one) user configs are not considered after all --> #21913

wardpeet added a commit to wardpeet/gatsby that referenced this pull request Mar 16, 2020
wardpeet added a commit that referenced this pull request Mar 16, 2020
* Revert "fix(gatsby-plugin-typescript): Broader webpack support (#22003)"

This reverts commit 4b93826.

* Revert "feat(gatsby-plugin-typescript): Supports linting (#18721)"

This reverts commit d28a0bd.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants