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

chore: Upgrades app to modern Typescript #375

Merged
merged 15 commits into from
Feb 8, 2024
Merged

chore: Upgrades app to modern Typescript #375

merged 15 commits into from
Feb 8, 2024

Conversation

markrickert
Copy link
Collaborator

@markrickert markrickert commented Feb 5, 2024

Summary

This PR converts the entire app to modern typescript. Along with this comes a lot of package upgrades now that react-diff-view and framer-motion support typescript

This addresses the Typescript support item in #108

Test Plan

  • yarn test runs and passes
  • I added a new yarn typecheck command and added it to the github workflow
  • I couldn't get the docker image up and running on my local M2 machine, so yarn test-e2e is failing locally for me.

Checklist

  • I tested this thoroughly
  • I added the documentation in README.md (if needed)

@pvinis
Copy link
Member

pvinis commented Feb 5, 2024

amazing! we've been wanting to move it to TS for a while now.

Allow me a couple of days because I'm on holidays, and I will review it on Wednesday 👍.

@markrickert
Copy link
Collaborator Author

No worries. The conversion to typescript found quite a few small bugs... typos, prop type mismatches, etc... so i fixed them! I've @ts-ignored the places I couldn't get the linter to pass so it might be worth visiting those places in the code.

Copy link
Member

@pvinis pvinis left a comment

Choose a reason for hiding this comment

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

looks great!

@pvinis
Copy link
Member

pvinis commented Feb 7, 2024

one tiny thing, i saw that the icons are not showing until hover.
Screenshot 2024-02-07 at 19 57 58
Screenshot 2024-02-07 at 19 58 04
fix that, and then we are good to go!

Copy link
Member

@pvinis pvinis left a comment

Choose a reason for hiding this comment

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

changing to avoid accidental merge

Copy link
Member

@pvinis pvinis left a comment

Choose a reason for hiding this comment

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

changing to avoid accidental merge

When running `yarn start`, the console would complain:

WARNING in ./node_modules/react-diff-view/es/index.js
Module Warning (from ./node_modules/source-map-loader/dist/cjs.js):
Failed to parse source map from ‘~/react-native-upgrade-helper/node_modules/react-diff-view/src/utils/parse.ts' file: Error: ENOENT: no such file or directory, open ‘~/react-native-upgrade-helper/node_modules/react-diff-view/src/utils/parse.ts'
@markrickert markrickert requested a review from pvinis February 8, 2024 04:21
@@ -1,5 +1,5 @@
import React, { Fragment } from 'react'
import { ReleaseT } from '../types'
import type { ReleaseT } from '../types'
Copy link
Member

Choose a reason for hiding this comment

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

is that needed? ts is smart enough to know this, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just a style preference. The TS compiler knows the difference.

While week on the topic, want are your thoughts on using the linter to enforce import order?

@pvinis
Copy link
Member

pvinis commented Feb 8, 2024

Nice, thank you! If there is nothing else here, I will merge in a couple hours.

@pvinis pvinis merged commit b6bbf79 into react-native-community:master Feb 8, 2024
1 check passed
@markrickert markrickert deleted the typescript branch February 8, 2024 16:56
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