-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat: update react & react-dom to v17 #338
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #338 +/- ##
=======================================
Coverage 78.26% 78.26%
=======================================
Files 34 34
Lines 667 667
Branches 172 172
=======================================
Hits 522 522
Misses 132 132
Partials 13 13 Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 question and a handful of nits.
packages/catalog-search/package.json
Outdated
"@fortawesome/react-fontawesome": "^0.1.4", | ||
"react": "^16.12.0", | ||
"react-dom": "^16.12.0", | ||
"@fortawesome/react-fontawesome": "^0.2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[curious] Why does this peer dependency need to be updated when ^0.1.4
should already allow 0.2.0
. Is there something in v0.2.0
that is necessary? Otherwise, this is technically a breaking change for any consumers that install < 0.2.0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mainly react-fontawesome
now supports forwardRef for version 0.2.x or above which is a breaking change for React version older than 16.3.0. Since we intend to merge this as a breaking change (mentioned in PR description), I updated this as well. Also "@fortawesome/react-fontawesome": "^0.1.4"
resulted in Unable to resolve path to module '@fortawesome/react-fontawesome'
which was also resolved with the update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mainly react-fontawesome now supports forwardRef for version 0.2.x or above which is a breaking change for React version older than 16.3.0.
[clarification] On master
, the minimum version of React specified in the repo's peerDependencies
is ^16.12.0
. Anything to do with React versions older than 16.3.0 should be irrelevant for this repository? For example, if the peer dependency is left as is at ^0.1.4
, a consuming MFE like frontend-app-learner-portal-enterprise can still upgrade to ^0.2.0
on their own without causing any issues since its minimum React version is beyond the impacted version.
Also "@fortawesome/react-fontawesome": "^0.1.4" resulted in Unable to resolve path to module '@fortawesome/react-fontawesome' which was also resolved with the update.
Hm, I can't replicate this issue on master
or this PR when installing 0.2.0
in devDeps
but left the peer dependency unchanged 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we intend to merge this as a breaking change (mentioned in PR description)
Because the peerDependency
is supporting both React 16 and 17 together, we're saying this library supports either React 16 or React 17. Introducing support for both 16 and 17 together in the peer dependency like this is not a breaking change. If we needed to remove React 16 from the peer dependency list, that would be a breaking change.
When some other libraries (e.g., Paragon) added support for React 17, it was added alongside React 16 and no major version / breaking change was released (example PR).
When it comes to upgrading React 18, that will definitely have issues where we'll have to likely drop support for React 16 (and possibly 17) at that time; IMHO only then should a React upgrade trigger the major version bump since we're saying consuming can run either React 16 or React 17 as is, which is not a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[clarification] On master, the minimum version of React specified in the repo's peerDependencies is ^16.12.0. Anything to do with React versions older than 16.3.0 should be irrelevant for this repository? For example, if the peer dependency is left as is at ^0.1.4, a consuming MFE like frontend-app-learner-portal-enterprise can still upgrade to ^0.2.0 on their own without causing any issues since its minimum React version is beyond the impacted version.
Yes, versions older than 16.3.0 would be irrelevant for this repo, I may have misinterpreted your first comment, my bad. What you said here makes sense, I have updated the peerDependency for @fortawesome/react-fontawesome
Hm, I can't replicate this issue on master or this PR when installing 0.2.0 in devDeps but left the peer dependency unchanged 🤔
There was a lint failure which you can find in actions here, thats if the devDep is set to ^0.1.4
, wasn't related to peerDependencies
… into bilalqamar95/react-upgrade-to-v17
ba26467
to
382b9d8
Compare
I have also updated paragon to the latest version, also we would like for this to go as a breaking change as mentioned in the description. |
packages/catalog-search/package.json
Outdated
"@fortawesome/free-solid-svg-icons": "5.8.1", | ||
"@fortawesome/react-fontawesome": "0.1.4", | ||
"@fortawesome/react-fontawesome": "^0.2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: most of our deps are pinned (i.e., remove the ^
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I have pinned the version
packages/catalog-search/package.json
Outdated
"@fortawesome/react-fontawesome": "^0.1.4", | ||
"react": "^16.12.0", | ||
"react-dom": "^16.12.0", | ||
"@fortawesome/react-fontawesome": "^0.2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mainly react-fontawesome now supports forwardRef for version 0.2.x or above which is a breaking change for React version older than 16.3.0.
[clarification] On master
, the minimum version of React specified in the repo's peerDependencies
is ^16.12.0
. Anything to do with React versions older than 16.3.0 should be irrelevant for this repository? For example, if the peer dependency is left as is at ^0.1.4
, a consuming MFE like frontend-app-learner-portal-enterprise can still upgrade to ^0.2.0
on their own without causing any issues since its minimum React version is beyond the impacted version.
Also "@fortawesome/react-fontawesome": "^0.1.4" resulted in Unable to resolve path to module '@fortawesome/react-fontawesome' which was also resolved with the update.
Hm, I can't replicate this issue on master
or this PR when installing 0.2.0
in devDeps
but left the peer dependency unchanged 🤔
packages/catalog-search/package.json
Outdated
"@fortawesome/react-fontawesome": "^0.1.4", | ||
"react": "^16.12.0", | ||
"react-dom": "^16.12.0", | ||
"@fortawesome/react-fontawesome": "^0.2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we intend to merge this as a breaking change (mentioned in PR description)
Because the peerDependency
is supporting both React 16 and 17 together, we're saying this library supports either React 16 or React 17. Introducing support for both 16 and 17 together in the peer dependency like this is not a breaking change. If we needed to remove React 16 from the peer dependency list, that would be a breaking change.
When some other libraries (e.g., Paragon) added support for React 17, it was added alongside React 16 and no major version / breaking change was released (example PR).
When it comes to upgrading React 18, that will definitely have issues where we'll have to likely drop support for React 16 (and possibly 17) at that time; IMHO only then should a React upgrade trigger the major version bump since we're saying consuming can run either React 16 or React 17 as is, which is not a breaking change.
Ticket
Upgrade React JS to v17
Description
Updated
react
&react-dom
to v17 along withtesting-library/react
&react-test-renderer
to respective compatible versionsBreaking Change
Upgrading React from version 16 to 17. This release includes breaking changes(the new JSX transform, changes to event handling, and deprecations of certain APIs), necessitating a major version increment. Eventually using the new JSX factory would be great but this PR focuses on bumping react to v17 along with respective packages.
Merge checklist:
frontend-app-learner-portal-enterprise
,frontend-app-admin-portal
, andfrontend-app-enterprise-public-catalog
). Will consumers safely be able to upgrade to this change without any breaking changes?BREAKING CHANGE
so the NPM package is released as such.Post merge:
chore(release): publish
) that incremented versions in relevant package.json and CHANGELOG files, and created Git tags for those versions.Publish from package.json
Github Action workflow to publish these new package versions to NPM.master
branch.npm view <package_name> versions --json
).