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

first stab at migrating to TypeScript #24

Merged
merged 8 commits into from
Jul 31, 2024
Merged

first stab at migrating to TypeScript #24

merged 8 commits into from
Jul 31, 2024

Conversation

raffazizzi
Copy link
Member

@raffazizzi raffazizzi commented Jul 24, 2024

@trevormunoz I migrated the code base to TypeScript. Dealing with the query results is... complex to put it mildly. I think we can identify better strategies to

  • produce a tighter graphql schema so that the resulting types are better defined (lots of nullable types that don't need to be so, for example, or inferred string | number types that can more firmly be one or the other.
  • come up with different strategies for merging data, e.g. associating people with their affiliations from a different table column

There's always benefits to using TypeScript, and I caught a couple of minor errors where the code looked up the wrong field, or where the field was missing. But dealing with these complex structures from AirTable may be more of a pain than needed.

Take a look if you can and see what you think!

NB the query types are generated the first time you run yarn develop npm run develop so make sure to do that before opening your IDE.

@raffazizzi raffazizzi requested a review from trevormunoz July 24, 2024 19:09
@trevormunoz trevormunoz mentioned this pull request Jul 24, 2024
@trevormunoz
Copy link
Member

trevormunoz commented Jul 29, 2024

This looks worth merging so we can take advantage of the Typescript for ongoing development. One question: are the names of the interfaces all automatically generated? (I only skimmed the docs on this.) I would be in favor of not having them all named just Props but rather something more descriptive.

For instance: https://github.com/umd-mith/mith-static/pull/24/files/6ec0d12646313f52403b86d98b57345f140b8cb1#diff-821f5f3657ec59efef7679018c9a8e3c99ff48220740d6c093f8a2bfa6fa77b9R7

@raffazizzi
Copy link
Member Author

The name of types for queries are generated automatically. For example query PeoplePast will generate a type called PeoplePastQuery.

On the other hand, I added interfaces called Props. They are limited to component files and they model the properties that each component will receive, so it seems a fairly self-explanatory name to me. We could do something like ComponentNameProps instead?

@trevormunoz
Copy link
Member

trevormunoz commented Jul 29, 2024 via email

This involved a bunch of flaililng until I followed links from a issue
to this merged PR:
https://github.com/gatsbyjs/gatsby/pull/36405/files#diff-9b4482a4b0c980ba8a14df389f19ca9f9cc347b0e9b2e2ea54e8c516e92cb27e

Make the option in gatsby-config an object and then there is a property
to generate types on build (which is needed to use in CI).

fix: typechecking needs some files from build

fix: Adjust npm typecheck script

fix: Run build not build-pages in typescript workflow

fix: add debugging step for types in GH Actions

fix: try again to build types in CI

fix: found the option to generate types for build 🫠

fix: whoops, deleted build step by accident
@trevormunoz
Copy link
Member

@raffazizzi Being able to run the type checker locally and in GitHub Actions (see cbdb0d6) caught a few additional errors: https://github.com/umd-mith/mith-static/actions/runs/10166800582

Copy link
Member

@trevormunoz trevormunoz left a comment

Choose a reason for hiding this comment

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

I think this is ready to merge

</div>
)
}
// TODO: The speakers list is not currently being displayed. Uncomment this code to display the list and also add to return statement below
Copy link
Member

Choose a reason for hiding this comment

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

One additional thing that Typescript checking turned up was a few places where we are going to the trouble of generating a chunk of a page but not returning/rendering it (probably because of data quality issues. I tried to mark these with some TODOs but I probably didn't do them all

@raffazizzi
Copy link
Member Author

Thanks for looking at this thoroughly, @trevormunoz. I'll merge for now and I will open a couple of issues for improving the code.

@raffazizzi raffazizzi merged commit 7257d1d into main Jul 31, 2024
2 checks passed
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.

2 participants