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

Bump react to 16 #344

Merged
merged 10 commits into from
Oct 25, 2023
Merged

Bump react to 16 #344

merged 10 commits into from
Oct 25, 2023

Conversation

rhystmills
Copy link
Contributor

@rhystmills rhystmills commented Oct 23, 2023

What does this change?

This PR bumps React to 16.14.0, which will allow us to make use of React packages that use hooks (introduced in 16.8.0) - for example the upcoming prosemirror-editor.

It also bumps Node from v18 to v20, and bumps a few related packages to sort out and incompatibilities.

PropTypes is moved to its own packages from React 16, which means I've had to update the import source in lots of files. componentWill methods are renamed UNSAFE_componentWill... in React 16+ - they're not unsafe in terms of introducing vulnerabilities but apparently encourage bad development practices.

react-dates needed to be bumped, requiring a new peer dependency ("react-with-direction"), and for css stylesheets to be imported directly, removing the previous scss files.

Changes to our linter requirements from the version bump to eslint-plugin-react meant a few changes to keep the linter happy, namely:

  • Adding to our ESLint conf so it knows the React version
  • Adding rel="noreferrer" to links with target="_blank"
  • Escaping some apostrophes and double quotes

I've also removed some packages that weren't mentioned anywhere in the program, specifically:

  • es6-promise
  • history
  • url-search-params
  • whatwg-fetch

How to test

Run the application locally using this branch, according to the instructions in the readme. Does it run as expected?

Run the tests (yarn test) - do they pass?

Base automatically changed from rm/bump-webpack-to-v5 to main October 24, 2023 13:35
@@ -1 +1 @@
18.18.2
20.8.1

Choose a reason for hiding this comment

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

Was going to point out that Node was bumped as part of this PR, but I think maybe you forgot to push this, or pushed to this branch instead? Just highlighting in case there were any other associated changes that weren't pushed in that PR, that you may want to bring in here.

Might be worth noting this node upgrade in the PR description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, looks like I pushed that to the wrong branch. I've updated the other PR description and will mention here as you suggest. Good spot!

Copy link

@samanthagottlieb samanthagottlieb left a comment

Choose a reason for hiding this comment

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

All tests pass! However when testing locally and getting to the point of publishing an atom I get the error 'Could not publish atom'. Have you come across this?

As this PR includes both Node and React upgrades I wonder if it's worth rebasing/merging with your previous webpack upgrade PR, then deploying to CODE to test?

@rhystmills
Copy link
Contributor Author

rhystmills commented Oct 24, 2023

All tests pass! However when testing locally and getting to the point of publishing an atom I get the error 'Could not publish atom'. Have you come across this?

As this PR includes both Node and React upgrades I wonder if it's worth rebasing/merging with your previous webpack upgrade PR, then deploying to CODE to test?

Definitely worth a look. That branch was the base for this one so I think the based may have been changed to main when it was merged. I'll check though!

@rhystmills
Copy link
Contributor Author

All tests pass! However when testing locally and getting to the point of publishing an atom I get the error 'Could not publish atom'. Have you come across this?

As this PR includes both Node and React upgrades I wonder if it's worth rebasing/merging with your previous webpack upgrade PR, then deploying to CODE to test?

I've not been able to replicate myself - seems to behave as the last branch did locally. Will publish to CODE and see how it does there.

@rhystmills
Copy link
Contributor Author

All tests pass! However when testing locally and getting to the point of publishing an atom I get the error 'Could not publish atom'. Have you come across this?
As this PR includes both Node and React upgrades I wonder if it's worth rebasing/merging with your previous webpack upgrade PR, then deploying to CODE to test?

I've not been able to replicate myself - seems to behave as the last branch did locally. Will publish to CODE and see how it does there.

Deployed to CODE and seems to be working as expected there, seems to have been a local development issue: https://atomworkshop.code.dev-gutools.co.uk/

@samanthagottlieb
Copy link

All tests pass! However when testing locally and getting to the point of publishing an atom I get the error 'Could not publish atom'. Have you come across this?
As this PR includes both Node and React upgrades I wonder if it's worth rebasing/merging with your previous webpack upgrade PR, then deploying to CODE to test?

I've not been able to replicate myself - seems to behave as the last branch did locally. Will publish to CODE and see how it does there.

Deployed to CODE and seems to be working as expected there, seems to have been a local development issue: https://atomworkshop.code.dev-gutools.co.uk/

Ran locally again and I'm able to publish an atom! Not sure why it wasn't working before. And all working well on CODE 👍

Copy link

@samanthagottlieb samanthagottlieb left a comment

Choose a reason for hiding this comment

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

Looks good!

@rhystmills rhystmills merged commit 50f4c92 into main Oct 25, 2023
1 check passed
@rhystmills rhystmills deleted the rm/bump-react branch October 25, 2023 10:29
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