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

Port JavaScript to TypeScript, Phase 1 #134

Merged
merged 28 commits into from
Dec 30, 2024
Merged

Port JavaScript to TypeScript, Phase 1 #134

merged 28 commits into from
Dec 30, 2024

Conversation

louh
Copy link
Member

@louh louh commented Dec 5, 2024

See #133.

This is about a day's work and stops just short of addressing some particularly tricky problems (and maybe they are only tricky for me and not someone with better knowledge of TypeScript + Gatsby). Thankfully Gatsby's build process is very forgiving and doesn't actually do strict type-checking before a build. You should see the type problems in a lint, though. It would be reasonable to break up future TypeScript work in follow-up PRs if we don't want to address it all at once.

Additional guidance / reference is here: https://www.gatsbyjs.com/docs/how-to/custom-configuration/typescript/

What's done

  • Added a barebones tsconfig.json to root. Its main purpose is to configure paths relative to ./lib for the TypeScript compiler so that imports do not have to be rewritten.
  • Update gatsby-config.js to automatically generate types for graphql queries to ./lib/gatsby-types.d.ts. There is likely more to be done here -- see below.
  • Added ./lib/index.d.ts to allow importing certain image assets.
  • A majority of the JS files in ./lib (excluding templates) have been ported to TypeScript. Most modules have been extremely cooperative. Some general points about strategy:
    • Components have been converted to React functional components where needed and typed as React.FC.
    • When needed, components have their prop types defined only when they have their own special props. If they use any props specific to Gatsby or React, prop types will extend from either PageProps (Gatsby) or React.PropsWithChildren (because usually the component needs children typed).
    • Remove usage of the prop-types package to define types
    • Remove usage of defaultProps in favor of default values
    • Styled components that take conditional parameters via props also have their own types, when needed.
  • UI mixins are special because the functions setSpace et al accept string arguments in a specific format. Instead of typing only as string, which is unhelpful, I started an approach where only acceptable literal string values are valid. This way, illegal values can be type-checked
  • Added @types/react-helmet to provide userland type definitions for the react-helmet package; this can be removed if Helmet is ever replaced by Gatsby Head.

Remaining issues (see also notes in relevant commits)

  • Gatsby assumes that all fields in GraphQL queries are nullable by default. This is annoying because TypeScript would then demand we handle all null values explicitly, which sucks and will make code less readable unless we can guarantee the fields are never null in the graphql schema. There is a way to do this but we'd probably have to set that up. Until then, rather than force components to handle nulls, I'm letting the type-check error slide.
  • The remainder of ./lib/templates need to be ported to TypeScript. I believe the main hurdle is the nullable graphql fields issue.
  • <Tile> is a polymorphic component that sometimes renders out an <a> instead of a
    . Right now there is a proof-of-concept to make it handle polymorphing, but it still doesn't handle the condition where if as="a" is passed as a prop, then the html attributes of <a> are allowed, and disallowed otherwise.
  • <Action> (and its sibling components) is tricky because when it is used, it can render a <Button>, a <Link>
    or it can be an obfuscated version of those (see withObfucscation). What it becomes is dependent on the combination of props and its values that passed to <Action>. I'm either not good enough at typescript to know what to do here or maybe we would have to revisit this approach????
  • setSpace currently fails type-checking because the string literal types are probably not properly defined.

Future work

  • When all is said and done:
    • Remove prop-types package
    • Remove ESLint rules pertaining to prop-types or default-props

louh added 28 commits December 5, 2024 09:33
also brandmark props should be optional bc they have default values
not done! see ts errors in setSpace.ts
not done; there are types to pass thru because <Tile> is polymorphic and
can be rendered as <a> with its associated props/attributes
this is a proof of concept for components that ingest graphql data.

graphql queries assume all fields are nullable by default. typescript would then
demand we handle all null values, which sucks and will make code less readable
unless we can guarantee the fields are never null in the graphql schema.
see here
https://www.gatsbyjs.com/docs/how-to/local-development/graphql-typegen/#non-nullable-types

in the case of people, we can guarantee certain fields, e.g. "html" is always present,
so is "frontmatter". fields like "bluesky" is definitely null-ish but i'm not sure if
it's actually null or just, say, empty string
i am kind of muddling through this attempt at making <Tile> work as a
polymorphic component because in some cases it renders out an <a> instead
of a <div>. right now it doesn't really cover the condition where if
`as="a"` is passed as a prop, then the html attributes of <a> are allowed
the problem here is that when <Action> is used, it can be a <Button> or a <Link>
or it can be an obfuscated version of those (see `withObfucscation`). What
it becomes is dependent on the combination of props and its values that passed to
<Action>. i'm either not good enough at typescript to know what to do here or
maybe we would have to revisit this approach????
@louh louh requested review from slifty and chriszs December 5, 2024 15:09
@slifty
Copy link
Member

slifty commented Dec 10, 2024

Sorry for my delay in reviewing this -- the family (including me) got hit with all the casual diseases at once!

Copy link
Member

@slifty slifty left a comment

Choose a reason for hiding this comment

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

thank you so much for taking this on, it looks great! My logic for reviewing a major refactor like this is to just make sure it passes CI!

(which is just to say, I glanced at the actual lines and it looked good, but I didn't dig incredibly deep since even if I noticed something that I had an opinion about I wouldn't wanna raise it!)

@louh
Copy link
Member Author

louh commented Dec 30, 2024

YOLO MERGE FROM SPAIN

@louh louh merged commit e4fb1a5 into main Dec 30, 2024
1 check passed
@louh louh deleted the louh/ts branch December 30, 2024 16:06
@chriszs
Copy link
Collaborator

chriszs commented Dec 30, 2024

Nobody expects the Spanish rash decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants