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.

Sign up for GitHub

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

Jump to bottom

[Umbrella] TypeScript Migration (Help wanted!) #21995

Closed
blainekasten opened this issue Mar 5, 2020 · 143 comments
Closed

[Umbrella] TypeScript Migration (Help wanted!) #21995

blainekasten opened this issue Mar 5, 2020 · 143 comments
Labels
help wanted Issue with a clear description that the community can help with. not stale topic: TypeScript Issues and PRs related to TS in general, public typings or gatsby-plugin-typescript

Comments

@blainekasten
Copy link
Contributor

blainekasten commented Mar 5, 2020 •
edited by LekoArts

Hello Community!

Gatsby is rewriting our core codebase in TypeScript! Checkout the RFC: #21798

We would love love love your help! This work is incredibly important in Gatsby being a stable product and you could be a part of it! We are looking for community members who would be willing to jump into the codebase in transition files from .js to .ts and add the proper typings. We have a few guidelines we are wanting to stick to when doing this:

Gatsby TS Guidelines

  • Only use named exports, no export default.
  • When importing a package, always try to find community maintained types instead of typing ourselves. Look at the DefinitelyTyped github repository.
  • Opt for using Interfaces for objects and functions.
  • Type aliases should be used for primitives, unions and tuples
  • Interface declaration merging is an anti-pattern and should not be used internally.
  • Use Array<type> rather than type[] syntax, as it's clearer and matches built-in types such as Map.
  • Use TypeScript's helper types, such as Record, rather than defining your own.
  • Every function should type its input and output (even if it’s inferrable)
  • Enums are great, but beware they map to index numbers if not redefined. This can cause weird problems for public APIs that interop to normal JS.
  • The any type can be used for stubbing module types that would be out of scope to migrate. But it should not be used for any functional input/outputs. The end goal of this migration is to have ZERO any types in the codebase. See if unknown makes more sense in that context. Don't use @ts-ignore comments.
  • Lastly and most imporantly, we want to have simple types whenever possible. Primarily we should try to avoid Generics. Generic code can be highly reusable, but that can also come with the greater chance of breaking things. If you do need to use generics, please either give them default values or ensure the generic value can be inferred. Code should be simple when possible. DRY is not always best.

If you're interested in getting involved, check out the list below. If you're interested in doing something, please respond as a comment, i'll put your name by a file so others know it's "claimed", when the PR is merged i'll check the box.

Tips to get involved: first read the contribution docs, and see how to set up your local environment. After you've claimed the file to work on, convert it locally and ensure there are no typing or lint errors displayed in your IDE. Run yarn typecheck and yarn lint:code in the root before opening the PR, which will show any errors. Once you're ready, open the PR and one of us will review it. Thanks, and good luck!

File list

  • packages/gatsby-page-utils
    • src/
      • /create-path.js + test
      • /ignore-path.js + test
      • /index.js
      • /validate-path.js + test
      • /watch-directory.js
@blainekasten blainekasten added help wanted Issue with a clear description that the community can help with. status: community assigned labels Mar 5, 2020
@blainekasten blainekasten self-assigned this Mar 5, 2020
@herecydev
Copy link
Contributor

I'll happily pick up gatsby-link. Been meaning to learn some more around there!

@blainekasten
Copy link
Contributor Author

@herecydev That sounds great! I'll put your name down! That one is relatively small and isolated, so feel free to do that whole package in one PR if you want.

@herecydev
Copy link
Contributor

@blainekasten just a few preliminary comments. There are quite a few globals/window variables: __BASE_PATH__, ___navigate, ___push, ___replace, __PATH_PREFIX__, ___loader. Is there a strategy about making those available to typescript? I believe they're all defined from within the gatsby package.

@blainekasten
Copy link
Contributor Author

@herecydev those variables are def meant to be like "private" "secret" variables.. So I don't think we'll expose any types from gatsby package as they are only meant to be used in a few places. I think the best course of action would just be to define types for them in the gatsby-link package.

@aminkhp
Copy link

aminkhp commented Mar 6, 2020

I'm excited to contribute, which package do you suggest for starting?

@blainekasten
Copy link
Contributor Author

@aminkhp awesome!! I would love it if we could just start chunking off individual files in PRs. Want to tackle gatsby-core-utils? I would love if you just did one file per PR as they are all very isolated.

@robertrbairdii
Copy link

I'd be happy to contribute some types. I'd be able to tackle gatsby-page-utils.

I took a peek at gatsby-core-utils by the way, and it looks like true-case-path is just an empty file. I'm not sure if that's intentional, but it might be worth deleting from the repo and removing from the list? https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-core-utils/src/true-case-path.js

@blainekasten
Copy link
Contributor Author

@robertrbairdii Great! If you could chunk up the work and submit 1 PR for each line I had, that would be fantastic.

Good point re the gatsby-core-utils. cc @aminkhp

@mottox2
Copy link
Contributor

mottox2 commented Mar 7, 2020 •
edited

I'd like to try src/commands/build-javascript.js.

ADDITION: I'd also like to tackle babel-preset-gatsby

@herecydev herecydev mentioned this issue Mar 7, 2020
Closed
@mottox2 mottox2 mentioned this issue Mar 7, 2020
Merged
@jlkiri
Copy link
Contributor

jlkiri commented Mar 7, 2020

I spent some time in src/redux/actions/public.js before, so I think I can help with it.

@robertrbairdii
Copy link

@blainekasten I was looking at the .babelrc while converting gatsby-page-utils and I noticed that there is no reference to @babel/plugin-transform-typescript in babel-preset-gatsby-package (https://github.com/gatsbyjs/gatsby/blob/master/packages/babel-preset-gatsby-package/index.js
). And running a test build, the converted .ts files don't make it to the /dist folder.

The babel config at the root of the repo has an override for it https://github.com/gatsbyjs/gatsby/blob/master/.babelrc.js#L12 And that config seems to be imported by other packages such as https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/babel.config.js

As far as I know, you can only pass the extensions as an argument through the CLI, so we would have to add the --extensions flag to each babel reference in the scripts section of all the package.json of the packages being converted. You could then either provide the override as is done in the root .babelrc or add @babel/plugin-transform-typescript in babel-preset-gatsby-package and allow values like isTSX to be configured via options passed in to the package.

I think this will block getting some of the TS conversions merged in and published.

@aminkhp aminkhp mentioned this issue Mar 7, 2020
Closed
@MichaelDeBoey MichaelDeBoey mentioned this issue Mar 7, 2020
Merged
@herecydev
Copy link
Contributor

@robertrbairdii I came to the same findings on my PR: #22027

It wasn't obvious at first, but lucky there is some prior art in the gatsby-cli project to look at. It might be worth a ticket to sweep through all the packages to support ts, which will make the files more approachable as a whole

@danielkov
Copy link
Contributor

I would love to help out and I don't have any preferences. Can you recommend me some parts to migrate?

@mottox2 mottox2 mentioned this issue Mar 7, 2020
Merged
@MichaelDeBoey
Copy link
Contributor

@danielkov Seeing by @blainekasten's comment (#21995 (comment)), I think you could start in gatsby-core-utils

@danielkov
Copy link
Contributor

@MichaelDeBoey I also have my eyes on gatsby-cli/src/structured-errors if nobody else is on it and then move onto gatsby-core-utils in another PR. Does that sound good?

@MichaelDeBoey
Copy link
Contributor

@danielkov That one's already taken by @LekoArts in #20597

@danielkov
Copy link
Contributor

I see #22031 is addressing one of the files of gatsby-core-utils. Should I wait for that merge, or start independently and resolve conflicts when done?

@MichaelDeBoey
Copy link
Contributor

@danielkov You can just start working on it.
The conflicts can be fixed before merging. 🙂

@mottox2 mottox2 mentioned this issue Mar 7, 2020
Merged
This was referenced Mar 7, 2020
Merged
Closed
@dimitropoulos dimitropoulos mentioned this issue Apr 6, 2021
Closed
25 tasks
@LekoArts LekoArts added topic: TypeScript Issues and PRs related to TS in general, public typings or gatsby-plugin-typescript and removed topic: TypeScript migration labels May 28, 2021
@AKASHNINJA
Copy link

I will work on packages/babel-preset-gatsby/index.js + test

@kartikcho kartikcho mentioned this issue Jun 17, 2021
Closed
@kartikcho
Copy link
Contributor

Any reason why packages/gatsby/src/utils/profile.js is completely commented out and if it has been unused since 2018, should we remove it altogether @blainekasten?

@alisson-suzigan
Copy link
Contributor

Hi guys, it's been a while since I've been around here.
I saw that the last attempt to migrate the schema/schema-composer.js to TS was a long time ago.
I opened a PR #31998 with this migration.

@alisson-suzigan alisson-suzigan mentioned this issue Jun 20, 2021
Merged
@ashiishme
Copy link
Contributor

I would like to work on src/schema/infer/add-inferred-fields.js

@A-Scratchy
Copy link

Are these still ongoing? I'd be happy to pick some up.

@lippytm
Copy link

lippytm commented Jul 11, 2021

Non-linear logic from dead body Space Aliens Ghostbusters Borg Super Artificial Intelligence NFT Crypto Time Machines Reruns Universes Reruns have been Repoed with the Harry Potter's Jumanji Educational Systems Reruns Universes Reruns and Upgradable; "But I"m usually dead 30+ years ago; but not always?

@AFEEFALISHAN
Copy link

hi

@nategiraudeau
Copy link
Contributor

@LekoArts can I work on
packages/gatsby/src/bootstrap/load-themes/index.js,
packages/gatsby/src/schema/extensions/index.js,
packages/gatsby/src/schema/infer/add-inferred-fields.js,
and packages/gatsby/src/schema/infer/index.js?

This was referenced Aug 12, 2021
Closed
Closed
Closed
@tonyhallett
Copy link
Contributor

tonyhallett commented Oct 19, 2021 •
edited

I was considering updating CreateResolverArgs from index.d.ts whilst we wait for the js allocations to be completed ? To type this correctly it is necessary to ask a question, one that is important for those performing schema migration.

For createResolvers the field configs provided are passed to type composer methods extendField ( or addFields but the intent is the former ). As such should the typing be the same as graph-ql compose with a change to the GatsbyResolver ( or a new WrappingGatsbyResolver with info typed for the originalResolver) ? Or is gatsby not exposing the thunks from graphql-compose given that there is no mention in the docs ?

  public extendField<TArgs = any>(
    fieldName: string,
    partialFieldConfig: Partial<
      ObjectTypeComposerFieldConfigAsObjectDefinition<TSource, TContext, TArgs>
    >
  ): this;

export type ObjectTypeComposerFieldConfigAsObjectDefinition<TSource, TContext, TArgs = any> = {
  type: ThunkWithSchemaComposer<
    ComposeOutputTypeDefinition<TContext> | Resolver<any, TContext, any>,
    SchemaComposer<TContext>
  >;
  args?: ObjectTypeComposerArgumentConfigMapDefinition<TArgs>;
  resolve?: GraphQLFieldResolver<TSource, TContext, TArgs>;
  subscribe?: GraphQLFieldResolver<TSource, TContext>;
  deprecationReason?: string | null;
  description?: string | null;
  extensions?: Extensions;
  [key: string]: any;
};

@Austaras Austaras mentioned this issue Dec 21, 2021
Closed
@forivall forivall mentioned this issue Jul 26, 2022
Closed
@Kornil Kornil mentioned this issue Jul 27, 2022
Merged
This was referenced Aug 4, 2022
Merged
Closed
Merged
@tom-raley
Copy link
Contributor

I'd be happy to take packages/gatsby/src/bootstrap/load-themes/index.js if this still needs doing!

@sophilophie
Copy link

Hello! I'd like to help out. What would be a good starting point?

@Kornil Kornil mentioned this issue Jan 28, 2023
Open
@jinxed515
Copy link

Hey! I'd like to work on this. Could you assign a file to me?

@LekoArts
Copy link
Contributor

We've migrated most of the files that are important to us to TypeScript. Thanks for all the help!

Since the start of this initiative we also wrote all new code in TypeScript. Some files that are still JS in our core codebase are a bit harder to migrate and we'll do it ourselves when we have the time and it's worth it.

Again, thanks all for your help :)

@gatsbyjs gatsbyjs locked as resolved and limited conversation to collaborators Apr 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
help wanted Issue with a clear description that the community can help with. not stale topic: TypeScript Issues and PRs related to TS in general, public typings or gatsby-plugin-typescript
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests