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.

Sign up for GitHub

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

Jump to bottom

[MPDX-8443] Upgrade TypeScript to the latest version #1194

Merged
merged 5 commits into from
Nov 18, 2024
Merged

[MPDX-8443] Upgrade TypeScript to the latest version #1194

merged 5 commits into from
Nov 18, 2024

Conversation

canac
Copy link
Contributor

@canac canac commented Nov 14, 2024

Description

  • Upgrade TypeScript to the latest version
    • Required upgrading yarn so that @types/jest would work properly
    • Switched from ^ version specifier to ~ since TypeScript doesn't follow semver
  • Regenerated yarn SDKs to use the current versions of eslint and TypeScript (I believe they're used for editor integration)
  • Fix all new TypeScript errors

MPDX-8443

Checklist:

  • I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
  • I have applied the appropriate labels. (Add the label "On Staging" to get the branch automatically merged into staging.)
  • I have requested a review from another person on the project

canac added 4 commits November 14, 2024 16:51
@canac
Upgrade TypeScript
5e56b9a 
Required upgrade to the latest yarn v3 and @types/jest packages to fix
type errors with describe/it/expect.
@canac
Update yarn SDKs
333dfaa
@canac
Fix trivial TypeScript errors
a8e7c7e
@canac
Ignore ergonomock field alias errors
c4b7e6b
@canac canac added the Preview Environment Add this label to create an Amplify Preview label Nov 14, 2024
@canac canac requested a review from dr-bizz November 14, 2024 22:56
@canac canac self-assigned this Nov 14, 2024
@github-actions GitHub Actions
Copy link
Contributor

Preview branch generated at https://8443-upgrade-typescript.d3dytjb8adxkk5.amplifyapp.com

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 14, 2024 •
edited
Loading

Bundle sizes [mpdx-react]

Compared against 95a5eae

No significant changes found

dr-bizz
dr-bizz reviewed Nov 15, 2024
View reviewed changes
Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

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

Reviewed the code. It looks great! thank you for upgrading TS. I'm waiting for it to rebuild so I can test the UI.
https://us-east-1.console.aws.amazon.com/amplify/apps/d3dytjb8adxkk5/branches/8443-upgrade-typescript/deployments#

.yarn/sdks/eslint/bin/eslint.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume you ran yarn self-update on these files in the commit "Update yarn SDKs". If you manually did it, you are a beast; I would struggle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha, no not manually. I ran yarn dlx @yarnpkg/sdks.

src/components/Dashboard/ThisWeek/TasksDueThisWeek/TasksDueThisWeek.test.tsx
@@ -80,7 +80,7 @@ describe('TasksDueThisWeek', () => {
subject: '1 the quick brown fox jumps over the lazy dog',
activityType: ActivityTypeEnum.PartnerCarePrayerRequest,
contacts: {
nodes: [{ hidden: true, name: 'Smith, Roger', id: '1' }],
nodes: [{ name: 'Smith, Roger', id: '1' }],
Copy link
Contributor

Choose a reason for hiding this comment

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

What error caused you to remove these and the other fields from tests on other pages? Was it that they were not set up correctly to begin with?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mock fields I removed like this one aren't present on the query. Now TypeScript complains about them. I'm removing them since they don't do anything anyway.

@dr-bizz
Copy link
Contributor

dr-bizz commented Nov 15, 2024

It seems like there is a deployment issue. It could just be this Preview

@dr-bizz dr-bizz added the On Staging Will be merged to the staging branch by Github Actions label Nov 15, 2024
@canac
Copy link
Contributor Author

canac commented Nov 15, 2024

It seems like there is a deployment issue. It could just be this Preview

@dr-bizz Thanks for pointing that out. It's a yarn configuration issue. I'll request another review when I have it fixed.

@canac canac requested a review from dr-bizz November 15, 2024 16:27
canac
canac commented Nov 15, 2024
View reviewed changes
amplify.yml
@@ -13,7 +13,6 @@ frontend:
fi
- echo "NEXTAUTH_URL=$NEXTAUTH_URL" >> .env
- echo "NODE_ENV=$NODE_ENV" >> .env
- yarn set version 3.3.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you look in the logs, yarn -v shows that yarn is correctly running version 3.8.6 after this change. Removing this line means one fewer places to have to update when we update yarn.

dr-bizz
dr-bizz approved these changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@dr-bizz dr-bizz 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

@canac canac merged commit 7574f9b into main Nov 18, 2024
18 checks passed
@canac canac deleted the 8443-upgrade-typescript branch November 18, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dr-bizz dr-bizz dr-bizz approved these changes

Assignees

@canac canac

Labels
On Staging Will be merged to the staging branch by Github Actions Preview Environment Add this label to create an Amplify Preview
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@canac @dr-bizz