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

chore(gatsby): Use Typescript internally! #19923

Merged
merged 5 commits into from
Dec 20, 2019
Merged

chore(gatsby): Use Typescript internally! #19923

merged 5 commits into from
Dec 20, 2019

Conversation

blainekasten
Copy link
Contributor

@blainekasten blainekasten commented Dec 2, 2019

Summary

This PR is the result of some exploration work to see what it would take to use TypeScript in gatsby source. This does not enable TypeScript for consumers. The goal of using TypeScript is to have the benefits of a type system to catch errors in our code paths that may not otherwise be easily found. This PR sets up the infrastructure to begin using TS and converts one file, gatsby/commands/develop. There is a lot of organizational and process oriented discovery work to still resolve, but they are not blockers for this.

What's next

With this PR we enable a slow migration of changing files to TS. The community could definitely get involved here! We'll need to decide some rules on how we want to approach typing our app before we can accept PRs. Things like, sharing types and what types we allow. Our goal is to simplify the types as much as possible, complex types usually mean complex code and a refactor is warranted.

@blainekasten blainekasten requested a review from a team as a code owner December 2, 2019 19:38
@blainekasten blainekasten force-pushed the idea/typescript branch 3 times, most recently from 1fc295e to a361d9b Compare December 2, 2019 22:39
@moonmeister
Copy link
Contributor

FYI: #13436

@blainekasten
Copy link
Contributor Author

@moonmeister Thank you for the prior art! This is still a bit of an exploration ticket, but the core team is happy to explore this more now!

@moonmeister
Copy link
Contributor

@blainekasten good to know, I think it would be huge for the project. Thanks for taking this on.

@blainekasten blainekasten force-pushed the idea/typescript branch 9 times, most recently from 2df0dee to 9aaa866 Compare December 12, 2019 19:04
@blainekasten blainekasten changed the title [WIP] Idea/typescript Use Typescript internally! Dec 12, 2019
@blainekasten blainekasten force-pushed the idea/typescript branch 2 times, most recently from d22e36f to ee4bf92 Compare December 12, 2019 19:53
report.panic(`There was a problem starting the development server`, err)
}
})
const listener = server.listen(program.port, program.host)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

typescript taught me that this callback function is not called with an error. But good news, we check for the port above and change the port to a new number. So this code was old and unused, but ts taught me to delete it!

@blainekasten blainekasten added effort: med type: maintenance An issue or pull request describing a change that isn't a bug, feature or documentation change labels Dec 12, 2019
@blainekasten blainekasten requested a review from a team December 12, 2019 20:36
Copy link
Contributor

@pvdz pvdz left a comment

Choose a reason for hiding this comment

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

My five cents

.babelrc.js Outdated Show resolved Hide resolved
packages/gatsby/src/commands/develop.ts Show resolved Hide resolved
packages/gatsby/src/commands/develop.ts Show resolved Hide resolved
packages/gatsby/src/commands/develop.ts Show resolved Hide resolved
@blainekasten blainekasten changed the title Use Typescript internally! chore(gatsby): Use Typescript internally! Dec 16, 2019
@blainekasten blainekasten force-pushed the idea/typescript branch 6 times, most recently from fcac80d to 3ca2f57 Compare December 17, 2019 15:27
@blainekasten blainekasten requested a review from a team December 17, 2019 15:55
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.

I've added some questions about guidelines and importing modules 👍 Thanks @blainekasten

.circleci/config.yml Outdated Show resolved Hide resolved
packages/gatsby-cli/src/index.ts Show resolved Hide resolved
packages/gatsby-cli/src/index.ts Outdated Show resolved Hide resolved
packages/gatsby-cli/tsconfig.json Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
tsconfig.json Show resolved Hide resolved
packages/gatsby/src/commands/develop.ts Show resolved Hide resolved
} = require(`../utils/webpack-error-utils`)
} from "../utils/webpack-error-utils"

type Cert = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to write down guidelines when to use type or interface? I personally use interface for everything except if I need to use union, intersection, ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I've had so many debates about this. There is no longer a practical difference besides syntax with types and interfaces. I'll switch to interfaces because that's more common in the typescript world. One benefit of interfaces is the practice of prefixing with I makes it obvious when importing whats a type and what's code.

e.g., import { Reporter, IReporter } from 'reporter';

Copy link
Contributor

Choose a reason for hiding this comment

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

I've had so many debates about this

Haha, I can imagine. Honestly, I don't care, I just think something written about our way of using typescript would be great. If an eslint rule exists I would also use it to keep consistency.

packages/gatsby/src/commands/develop.ts Outdated Show resolved Hide resolved
peril/rules/merge-on-green.ts Outdated Show resolved Hide resolved
remove flow from eslint

Add typings for `got`

fix lint

drop commit test:check

bring back webpack-utils

Allow flow to live with typescript

fix lint

Improve check-ts script and add ts transformation to a second package

Fix ts imports

Fix gatsby-cli build script

address PR comments
Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

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

Let's do this. Great work @blainekasten ❤️

@sidharthachatterjee sidharthachatterjee added bot: merge on green Gatsbot will merge these PRs automatically when all tests passes and removed status: inkteam to review labels Dec 20, 2019
@gatsbybot gatsbybot merged commit 416afdb into master Dec 20, 2019
@delete-merged-branch delete-merged-branch bot deleted the idea/typescript branch December 20, 2019 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes type: maintenance An issue or pull request describing a change that isn't a bug, feature or documentation change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants