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): Expose typescript transpiler to default site in plugin list #26452

Merged
merged 2 commits into from
Sep 22, 2020

Conversation

Js-Brecht
Copy link
Contributor

@Js-Brecht Js-Brecht commented Aug 14, 2020

Description

Provides an option to disable the automatic set up of gatsby-plugin-typescript.

When gatsby-plugin-typescript is enabled, it disallows the use of any other Typescript transpiler (with type-checking enabled). This means that using something like ts-loader with Typescript transformer plugins is impossible. In order to allow this again, gatsby-plugin-typescript needs to be disabled.

There were a few options I could see for achieving this:

  1. Move the logic that adds gatsby-plugin-typescript to the plugin array above default-site-plugin.

    • 👍 Easiest method for developer.
    • 👍 Doesn't add any more surface area to the gatsby-config api.
    • 👎 While this would allow the default site to filter that plugin out, if it's desired, that process can be laborious and error prone.
    • 👎 Puts the onus on the end-user to reverse core setup.
  2. Webpack config post-processing (i.e. after onCreateWebpackConfig)

    • 👍 Automatically handle typescript loaders, no matter where they're added
    • 👎 Very error prone. Likely impossible to account for all circumstances
  3. Add a new "plugin filter" or "webpack loader filter" API

    • 👍 More future proof
    • 👎 Seemed like a pretty major API change
  4. Add a new configuration value to gatsby-config.

    • 👍 Fixes the issue at the root
    • 👍 End-user experience is much, much nicer
    • 👎 Increases the surface area of the gatsby-config api.

I opted for number 4 because it just made the most sense to me.

  • The end-user experience is much better than the others.
    • Why force the user to filter out a loader in the webpack config. It's counter-intuitive to add loaders, then turn around and remove them. Means more moving parts that don't really need to be there.
  • The default behavior remains the same as originally designed. Unless people need it, they can continue on without ever caring that the feature exists.
  • Implementation itself was not difficult, nor is the process difficult to understand.

Documentation

docs/gatsby-config.md

Related Issues

Fixes #26027

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Aug 14, 2020
@vladar vladar added topic: TypeScript Issues and PRs related to TS in general, public typings or gatsby-plugin-typescript status: needs core review Currently awaiting review from Core team member and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Aug 14, 2020
@Js-Brecht
Copy link
Contributor Author

@vladar do you know who I can talk to to try to get some attention on this? With the new paradigm, things are a bit more...opaque here. This issue is causing me quite a few headaches, so it would be great to get some sort of solution to it.

Copy link
Contributor

@vladar vladar left a comment

Choose a reason for hiding this comment

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

Hey @Js-Brecht !

Thanks for suggesting this 👍 The code looks good solid me.

I think you are right and we should allow to opt-out of internal plugins like Typescript. I guess the main question here is if a new config option is a way to go here. Makes sense to me but I'd like to hear other opinions before we merge this.

@vladar vladar self-assigned this Sep 14, 2020
@vladar vladar changed the title feature(gatsby) Option to disable default typescript transpiler feat(gatsby): Option to disable default typescript transpiler Sep 14, 2020
@Js-Brecht
Copy link
Contributor Author

Yeah, I wasn't really sure where else it would go.

One other option I didn't really consider was an environment variable; but this is something that would generally be a permanent setting, as opposed to something that you would switch on/off. If I needed to rely on an environment variable in a project where I wanted to turn off the default typescript transpiler, I would probably wind up doing checks for it (or setting it) in gatsby-config anyway, since the build would be so reliant on it.

@vladar
Copy link
Contributor

vladar commented Sep 14, 2020

Another possible option is to disable the plugin this way:

{
  resolve: `gatsby-plugin-typescript`,
  options: {
    disable: true
  }
}

It is more explicit and also can be applied to other internal plugins consistently. Do you see any pitfalls with this approach?

@Js-Brecht
Copy link
Contributor Author

Js-Brecht commented Sep 14, 2020

Do you see any pitfalls with this approach?

I could see how requiring another dependency be installed that you're not really using could be annoying. It's included in Gatsby's dependencies now, but when using other package managers like pnpm or yarn2, you'd have to also have it installed in your project's deps or the build will fail.

Not really a blocker, and since it's a one-time setup it wouldn't be any real overhead.

One way around that would be to skip resolution for "disabled" internal plugins. That adds another layer of complexity to the plugin/theme resolution that I don't really think this issue warrants, though 🤷‍♂️

@wardpeet
Copy link
Contributor

wardpeet commented Sep 15, 2020

Move the logic that adds gatsby-plugin-typescript to the plugin array above default-site-plugin

This seems like the easiest solution, right now.

Expanding the config to enable/disable pieces of gatsby seems like a good approach but it takes planning and quite some resources to get it right. Something I don't think we'll going to prioritize. If we add typescript to the config like you did, it means we have to support this for a while... (we cans say yes but we don't know what issues it might bring in the future)

How do we feel about:

{
  resolve: `gatsby-plugin-typescript`,
  options: {
    typecheck: true
  }
}

When typecheck is true we will use ts-loader instead of babel-preset-typescript. (I don't know if it's that simple)

@Js-Brecht
Copy link
Contributor Author

How do we feel about:

{
  resolve: `gatsby-plugin-typescript`,
  options: {
    typecheck: true
  }
}

When typecheck is true we will use ts-loader instead of babel-preset-typescript. (I don't know if it's that simple)

I like that idea; it would almost be that simple. Since Typescript transpiling would then rely on tsconfig.json, it would make sense to also include some default options that are required for it to be successful. gatsby-plugin-typescript would also need to take all of the potential options that ts-loader does, since ts-loader is quite extensible.

If this is the route that's decided, I would have a couple questions regarding implementation:

  • Since this is a core plugin, what are your thoughts about structuring plugin options? I generally prefer namespacing options that go to a particular loader, and leave the base options to control the plugin. i.e.

    options: {
      typecheck: true, // <-- base options
      tsLoader: { // <-- namespaced loader options
        getCustomTransformers: (program) => // etc...
        compilerOptions: {
          /* Override tsconfig options */
        }
      }
    }
    • If the plugin begins allowing options like that, I wonder if it should also allow options that will overload babel-loader options.

      options: {
        babelLoader: {
          /** Some babel loader options */
        }
      }
  • Would it make sense to add ts-loader to loaders (like loaders.ts())? That would indicate to me that it would be a dependency of Gatsby itself. If not, then I think it would be installed in gatsby-plugin-typescript instead.

  • Since Gatsby will require some tsconfig settings to transpile successfully, I think it would also make sense to include some documentation about what those requirements are somewhere. That way, if the user overrides them, they know why if something breaks. Any thoughts on where that would go?

I would also move the default inclusion above default-site-plugin, too, so that it can still be stripped out by the user as a last resort.

@wardpeet
Copy link
Contributor

wardpeet commented Sep 18, 2020

@Js-Brecht let's start with moving the typescript plugin higher in the chain so you can at least remove it yourself. Could you open a new issue with this discussion as I think we'll have to think it through.

What problems we could see on the way with every approach. I'll put some effort to move this forward.

@Js-Brecht
Copy link
Contributor Author

Okay, gatsby-config changes reversed and gatsby-plugin-typescript moved before default-site-plugin

@Js-Brecht Js-Brecht changed the title feat(gatsby): Option to disable default typescript transpiler feat(gatsby): Expose typescript transpiler to default site in plugin list Sep 21, 2020
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.

Awesome! Thanks for the back and forth!

@wardpeet wardpeet merged commit a8ce6e6 into gatsbyjs:master Sep 22, 2020
@Js-Brecht Js-Brecht deleted the disable-typescript-option branch September 22, 2020 17:37
@wardpeet
Copy link
Contributor

Successfully published:

  • gatsby@2.24.64

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs core review Currently awaiting review from Core team member topic: TypeScript Issues and PRs related to TS in general, public typings or gatsby-plugin-typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Typescript by default" interferes with ts-loader in Webpack build
3 participants