-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
feature(gatsby): Support TypeScript by default by compiling TS files #23547
Conversation
|
||
let plugins = await loadPlugins(config) | ||
|
||
plugins = replaceFieldsThatCanVary(plugins) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to create a different helper function here? The one that exists now also check paths property and might cause weird test failures when we might move more plugins as a default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. Do you have any suggestions? It seems to work right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the same setup we have for gatsby-source-page-creators
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe replaceFieldsThatCanVary
should also just do more checks before trying to do pluginOptions.path
sanitization (apply it only for gatsby-plugin-page-creator
)?
But this doesn't seem to be concern of this pull request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! I would ship this PR without any of the modifications I'm going to list here as it's more tech depth and might complicate the PR review a bit more.
Maybe in a follow-up PR we could clean up the load-plugins function a bit.
- Refactor config.plugins check to only happen once. Instead of x amount of times. I would slap a default
[]
on it. - extract the require.resolve and plugin find check into a helper function so it can be re-used.
- Move default plugins to an array so we can loop through it instead of duplicating these code paths.
@wardpeet great idea, I created a clubhouse ticket to follow up on that refactor work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great -- we'll definitely want to document this (gatsby-plugin-typescript documentation should change), and I honestly think this is shoutable with a blog post, as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, this is exciting!
@wardpeet But is this something that we will actually ever do? We do plugins iterations almost any time we call Is the extra abstraction worth doing here? Current code is pretty straight forward I think and arguably might be easier to reason with than the added utilities which would have their own API etc |
Actually, we should likely make some docs adjustments in this PR as well before merging this in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think code-wise this is good to go. However we likely need some documentation updates as well?
Documentation should also include the note that you need to add it to your own gatsby-config.js so that you add the options that the plugin accepts. A "Using TypeScript" doc then would only prompt you do init a tsconfig and install typescript + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great. Thanks for talking with me about how to document this change, @blainekasten! I've left a few comments. Could you also add the TypeScript doc to the sidebar under Reference Guides? You'll need to add it to doc-links.yaml
.
Everything that follows are suggestions and shouldn't block this PR. For the reference guide, I'd call out that people interested in trying TypeScript don't need to switch their whole app over all at once. (I've never actually done this on an app in production. You're a much better person to guide folks on how to move an existing app to TypeScript.) I'd also talk a little bit about why someone might want to use TypeScript with their Gatsby site. This needn't be more than a sentence or two. I would imagine it could do double duty and also touch on why we're supporting TypeScript by default.
Co-authored-by: Aisha Blake <[email protected]>
Co-authored-by: Aisha Blake <[email protected]>
…/gatsbyjs/gatsby into typescript-by-default/compile-ts
@AishaBlake I gave it another pass and included all of your suggestions. Want to take another gander? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tweaked the wording of one sentence to use "you" as the pronoun per the style guide. This looks good!
|
||
## Introductory paragraph | ||
|
||
[TypeScript](https://www.typescriptlang.org/) is a JavaScript superset which extends the language to include type definitions allowing codebases to be statically checked for soundness. Gatsby provides an integrated experience out of the box, similar to an IDE. If you are new to TypeScript, adoption can _and should_ be incremental. Since Gatsby natively supports JavaScript and TypeScript, you can change files from .js to .tsx at any point to start adding types and gaining the benefits of a type system. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥 Yes! This hits all the points I had in mind!
If you are new to TypeScript, check out these other resources to learn more: | ||
|
||
- [TypeScript Documentation](https://www.typescriptlang.org/docs/handbook/basic-types.html) | ||
- [TypeScript Playground (Try it out!)](https://www.typescriptlang.org/play/index.html) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gatsby's API is available in the Playground, so this could be more like this link instead
BTW. Since you make TS as a first-class citizen etc. Please take a look and address this issue. Integrating GraphQL Codegen into Gatsby or making an official plugin for it will be an awesome thing too because that will make Gatsby e2e typed automatically. Please address that if you really want THE BEST dev experience for Typescript projects. |
gatsby-plugin-typescript is included in Gatsby's default configuration see gatsbyjs/gatsby#23547
…atsbyjs#23547) * feature(gatsby): Support TypeScript by default by compiling TS files * add a test to ensure we arent adding an extra gatsby-plugin-typescript * Make a TypeScript file in gatsby-starter * Add TypeScript example to blog starter * Update Using Typescript example * update gatsby-plugin-typescript README.md * Add Basic TypeScript example * Update packages/gatsby-plugin-typescript/README.md Co-authored-by: Aisha Blake <[email protected]> * Update packages/gatsby-plugin-typescript/README.md Co-authored-by: Aisha Blake <[email protected]> * Add more to the TS docs * Add docs to sidebar * Use you as the pronoun * Fix e2e test Co-authored-by: Aisha Blake <[email protected]>
Description
🎉We are working on supporting TypeScript as a first class citizen within Gatsby 🎉
We already have the
gatsby-plugin-typescript
plugin that adds compilation support for.ts(x)
files. We've decided that as we add more support for TypeScript, we want to do it via the plugin. So our first step is to make that plugin part of the default gatsby set up.This PR does the following:
gatsby-plugin-typescript
to the loaded plugins with no custom options if not provided by the users gatsby-config.gatsby-config
if it includes agatsby-plugin-typescript
entry.Documentation
Docs exist here: https://www.gatsbyjs.org/packages/gatsby-plugin-typescript/
Do we want to add some banner that this is auto-included in gatsby and that customization can be done by adding it to their config?
Related Issues
#18983