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

Convert most remaining JS to TS #5043

Merged
merged 6 commits into from
Sep 12, 2024
Merged

Convert most remaining JS to TS #5043

merged 6 commits into from
Sep 12, 2024

Conversation

Vinnl
Copy link
Collaborator

@Vinnl Vinnl commented Sep 11, 2024

References:

Jira: MNTOR-3077
Figma:

Description

There were a couple of remaining files in /src/utils that were still plain JS, and some in /src/db/. This PR converts them to TS, which is possible now that our cron jobs are TS as well.

The remaining JS is:

  • localStorage.js - this can be removed, but since that conflicts with Breach alert email redesign #5012, I'm holding off on that for now.
  • DB migrations - I don't think Knex supports these as TS, they're fairly trivial anyway, and the existing scripts shouldn't ever change either, so I'm leaving these.
  • AppConstants - that has more impact, so I have that as a separate PR in case people have feelings about that: Remove AppConstants #5044
  • Some config files at the root, which should probably stay JS.
  • Scripts in /src/scripts/loadtest/ and /src/scripts/build/; introducing the TS toolchain in there is probably more of a nuisance than a help.

So, give or take a couple of anys still lying around the codebase, mostly brings us to full type safety in the entire codebase!

@Vinnl Vinnl added the Review: XS Code review time: up to 30min label Sep 11, 2024
@Vinnl Vinnl self-assigned this Sep 11, 2024
@Vinnl Vinnl mentioned this pull request Sep 11, 2024
Copy link

Copy link
Collaborator

@rhelmer rhelmer left a comment

Choose a reason for hiding this comment

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

It's great to finally have this done!

I believe that knex supports TS for migration scripts now, but I agree that it's probably going to be minimally useful, especially for the older migration files that are mostly for record-keeping purposes. However note that we do run these still for new installs (mostly dev environments) when npm run db:migrate is run for the first time on a new database, so I think it would be OK to convert these if we ever want to.

I agree as well that config files are likely to benefit the least from type safety, I suspect we could move most or all of these but it's not very important.

@Vinnl
Copy link
Collaborator Author

Vinnl commented Sep 12, 2024

However note that we do run these still for new installs (mostly dev environments) when npm run db:migrate is run for the first time on a new database, so I think it would be OK to convert these if we ever want to.

Yeah, but that's exactly why I'm weary about converting them: we want those to run exactly the same as they did for us, but since we've already run them, if I make any errors while converting, we wouldn't catch that. (I don't think I'd make mistakes, since they're pretty trivial and there should be no runtime changes anyway, but you never know.) And since no other code by definition depends on the migrations, there's not really any benefit to converting them anyway. Could be something to consider for future migrations though.

@Vinnl Vinnl merged commit b0d848b into main Sep 12, 2024
15 checks passed
@Vinnl Vinnl deleted the MNTOR-3077-everything-ts branch September 12, 2024 10:15
Copy link

Cleanup completed - database 'blurts-server-pr-5043' destroyed, cloud run service 'blurts-server-pr-5043' destroyed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review: XS Code review time: up to 30min
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants