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

Minimal typescript migration #84

Closed
wants to merge 5 commits into from

Conversation

TomAFrench
Copy link
Contributor

@TomAFrench TomAFrench commented Jun 1, 2022

This PR performs an MVP migration of loot-core over to typescript:

  • A TS compatible linter has been set up.
  • ts-loader has been added to the webpack config so that it can load TS files
  • The entrypoint file main.js has been replaced by main.ts.
  • The minimum possible changes have been made to the source files to fix the resultant TS errors and allow the loot-core package to build.

This allows us to gradually migrate the rest of the package over to typescript without having to do everything at once (causing huge merge conflicts with the other PRs). Considering how widely loot-core is used across the other packages, I think it makes sense to migrate this package first and once it has a well defined interface then we can migrate the other packages over to TS.

One odd thing I've noticed is that we've set the context for the loot-core package to be the monorepo root which seems to cause a need for ts-loader to be installed up there (or at the very least hoisted). I haven't looked into whether this is necessary or if we can remove this.

@TomAFrench TomAFrench marked this pull request as draft June 27, 2022 23:47
@TomAFrench
Copy link
Contributor Author

Converting to draft as this currently blows up running tests in loot-core as babel-jest can't handle TS. The plan is to rebase this on top of TomAFrench@28056ea down the line once that's in master.

@jcdickinson
Copy link

Have you considered renaming everything to .ts (with git mv for full merge fidelity)? TS is a strict superset of JS, so every file in this repo is already valid TS.

@TomAFrench
Copy link
Contributor Author

TomAFrench commented Jul 21, 2022

Doing that would destroy information on which files have had a meaningful migration over to typescript. We'd also be requiring those other files to pass the type checks which they likely would fail (main.ts failed a number of checks which required me to modify several other files). We'd then need to do extra work to add in the minimal changes to get these checks to pass.

It'd also add a ton of noise to the PR so if we were to do that I'd prefer to do it as a separate PR, my preference though is to migrate to .ts directory by directory as we add strict types to those files.

@MatissJanis
Copy link
Member

Closing this PR as stale. Someone else can pick up the work started here and continue it in a new PR.

#577

@j-f1 j-f1 mentioned this pull request Feb 13, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs testing This needs testing
Projects
Status: Fixed
Development

Successfully merging this pull request may close these issues.

4 participants