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: upgrade yarn to v4 and other dependencies #5014

Merged
merged 17 commits into from
Aug 30, 2024

Conversation

abnud11
Copy link
Contributor

@abnud11 abnud11 commented Jul 29, 2024


name: Upgrade dependencies of root project
about: it's about upgrading deps of the project
title: "Upgrade dependencies of root project"
labels: ""
assignees: "abnud1"

Description

Problem
This PR upgrades yarn from v1 to v4 using corepack, it also upgrades other deps to the latest version, especially major updates like react-testing-library, Typescript from 5.4 to 5.5, and others.

This PR also upgrades CodeQL Github action to use v3.
Changes

I updated package.json along with yarn.lock, codeql-analysis.yml file, I also added @types/react and @types/react-dom and enabled skipLibCheck: true in tsconfig in order to make yarn type-check script successful

Contribution checklist

  • I have followed the contributing guidelines.
  • I have added sufficient test coverage for my changes.
  • I have formatted my code with Prettier and checked for linting issues with ESLint for code readability.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

✅ This pull request was sent to the PullRequest network for review. Expert reviewers are now being matched to your request based on the code's requirements. Stay tuned!

What to expect from this code review:
  • Comments posted to any areas of potential concern or improvement.
  • Detailed feedback or actions needed to resolve issues that are found.
  • Turnaround times vary, but we aim to be swift.

@abnud11 you can click here to see the review status or cancel the code review job.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

PullRequest Breakdown

Reviewable lines of change

+ 114
- 72

58% JSON
26% GitHubActions
8% GitHubActions (tests)
5% TSX (tests)
4% Other

Generated lines of change

+ 37,863
- 26,487

Type of change

Feature - These changes are adding a new feature or improvement to existing code.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

The yarn and dependency updates look good from my perspective and seem like they should work as long as the build completes successfully and tests are passing. No concerns here. Thanks!

Image of Jonah Jonah


Reviewed with ❤️ by PullRequest

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

PullRequest Breakdown

Reviewable lines of change

+ 95
- 67

66% JSON
30% GitHubActions
2% Other
2% JSON with Comments
1% YAML

Generated lines of change

+ 37,683
- 26,447

Type of change

Feature - These changes are adding a new feature or improvement to existing code.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

PullRequest Breakdown

Reviewable lines of change

+ 95
- 67

66% JSON
30% GitHubActions
2% Other
2% JSON with Comments
1% YAML

Generated lines of change

+ 37,683
- 26,447

Type of change

Feature - These changes are adding a new feature or improvement to existing code.

@abnud11
Copy link
Contributor Author

abnud11 commented Aug 1, 2024

After upgrading react-testing-library to v16, some tests failed.

For some reason, fireEvent.keyDown is not firing sometimes, the resolution to those cases are documented here:
https://testing-library.com/docs/guide-events#keydown

In short, you need to focus on the element to dispatch keydown, then dispatch keydown on document.activeElement || document.body, while this should be done in all the cases where fireEvent.keyDown is used, I used it only in the failing cases, not sure why it succeeds sometimes and fails other times.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

Changes look good. Thanks for cleaning up.

Image of Luciano C Luciano C


Reviewed with ❤️ by PullRequest

@@ -1251,7 +1251,8 @@ describe("DatePicker", () => {
expect(formatDate(data.instance.state.preSelection!, data.testFormat)).toBe(
formatDate(data.copyM, data.testFormat),
);
fireEvent.keyDown(selectedDayNode!, getKey(KeyType.ArrowRight));
(selectedDayNode as HTMLElement).focus();
fireEvent.keyDown(document.activeElement || document.body, getKey(KeyType.ArrowRight));
Copy link

Choose a reason for hiding this comment

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

Good fix with this, checking the body in addition to the element for the keydown effect 👍

◽ Compliment

Image of Luciano C Luciano C

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

Change from last time is library bumps only. Nothing more to add.

Image of Luciano C Luciano C


Reviewed with ❤️ by PullRequest

@abnud11
Copy link
Contributor Author

abnud11 commented Aug 16, 2024

Can we approve the workflows and merge this PR please ?

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

Due to inactivity, PullRequest has cancelled this review job. You can reactivate the code review job from the PullRequest dashboard.

@abnud11
Copy link
Contributor Author

abnud11 commented Aug 28, 2024

Fix merge conflicts and prettier error

The workflows should succeed now.

Copy link

codecov bot commented Aug 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.85%. Comparing base (c834746) to head (ca934b8).
Report is 18 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5014   +/-   ##
=======================================
  Coverage   96.85%   96.85%           
=======================================
  Files          29       29           
  Lines        3343     3343           
  Branches     1390     1404   +14     
=======================================
  Hits         3238     3238           
+ Misses        105      103    -2     
- Partials        0        2    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +58 to +59
"@types/react": "^18.3.4",
"@types/react-dom": "^18.3.0",
Copy link
Member

Choose a reason for hiding this comment

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

These 2 are new? Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yarn type-check which is part of Github test workflow fails without these 2 packages, honestly don't know why it didn't error before.

package.json Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Why is this removed?

Copy link
Contributor Author

@abnud11 abnud11 Aug 30, 2024

Choose a reason for hiding this comment

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

This is part of husky 9 upgrade, while husky was on version 9 before this PR, this change should have been done when migrating from 8 to 9, see this:
https://github.com/typicode/husky/releases/tag/v9.0.1

In particular, read How To Migrate
@martijnrusschen

@martijnrusschen
Copy link
Member

Ok thanks let's resolve the last conflict and merge this.

@abnud11
Copy link
Contributor Author

abnud11 commented Aug 30, 2024

@martijnrusschen conflicts fixed.

@martijnrusschen martijnrusschen merged commit 71cc9e8 into Hacker0x01:main Aug 30, 2024
6 checks passed
@martijnrusschen
Copy link
Member

Something is wrong with Eslint. I can't get it to work anymore. It seems to consistently fail now, and as you can see in this PR, the linter didn't run but did succeed.

@martijnrusschen
Copy link
Member

Same seems to happen with sass-lint

@abnud11
Copy link
Contributor Author

abnud11 commented Sep 1, 2024

@martijnrusschen it works on my machine correctly, what exactly doesn't work on your machine? did you run yarn install?

@abnud11
Copy link
Contributor Author

abnud11 commented Sep 1, 2024

also, the linter ran as part of update (20.x) workflow

@abnud11
Copy link
Contributor Author

abnud11 commented Sep 1, 2024

And sass-lint works too

@martijnrusschen
Copy link
Member

@abnud11
Copy link
Contributor Author

abnud11 commented Sep 2, 2024

@martijnrusschen that job fails at yarn test:ci not yarn lint, yarn lint actually passes and I don't think the test failure is related to this PR.

@martijnrusschen
Copy link
Member

but if you compare it to an older run, let's say this one: https://github.com/Hacker0x01/react-datepicker/actions/runs/10460719357/job/28967495012. you will see more output for the lint job. It should run 2 linters, and it doesn't seem to run normally.

@abnud11
Copy link
Contributor Author

abnud11 commented Sep 3, 2024

That's because it was using yarn v1, with yarn v4 it doesn't show what commands it runs, only their output if there is any output, eslint and stylelint doesn't show any output unless there is an error.

I confirmed on my local machine that eslint catches errors by introducing errors in Typescript files, I also ensured that stylelint catches errors by introducing errors to sass code too.
Without introduced errors, they don't output anything since there are no errors.

@martijnrusschen

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