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

React 19 update #1632

Merged
merged 69 commits into from
Jan 24, 2025
Merged

React 19 update #1632

merged 69 commits into from
Jan 24, 2025

Conversation

ThomasGross
Copy link
Contributor

@ThomasGross ThomasGross commented Jan 7, 2025

Link to issue

https://reload.atlassian.net/browse/DDFBRA-343

Additional afflicted tickets:

https://reload.atlassian.net/browse/DDFBRA-312
https://reload.atlassian.net/browse/DDFNEXT-261

Description

This PR updates React to version 19 and many dependencies that require updates because of that.

Copy link
Contributor

@kasperg kasperg left a comment

Choose a reason for hiding this comment

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

👍 Appreciate all your updates into this. We have gone far but there is still a bit of way.

Three important things worth highlighting:

  1. We need to update the documentation regarding code guidelines. This has explicit references to Airbnb. I am considering whether this even needs an ADR describing the (updated) choice of coding standards.
  2. We have an recurring issue about react/display-name that we need to figure out what to do about. It seems to be specifically related to our use of Higher-Order Components. To me the reasoning behind the rule makes sense: "This name is used by React in debugging messages". If we do not appreciate this - or there is generally is no quick fix for this - then we should consider disabling the rule altogether.
  3. The situation about useIntersection() seems to be unresolved. Let's talk about that.

Comment on lines +53 to +70
eslint:
patterns:
- "eslint"
- "eslint-config-prettier"
- "eslint-loader"
- "eslint-plugin-cypress"
- "eslint-plugin-import"
- "eslint-plugin-jsx-a11y"
- "eslint-plugin-no-only-tests"
- "eslint-plugin-prettier"
- "eslint-plugin-react"
- "eslint-plugin-react-hooks"
- "eslint-webpack-plugin"
- "@eslint/*"
cypress:
patterns:
- "cypress"
- "@cypress/*"
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Nice. These are meaningful groups to add.

Comment on lines 24 to 26
"src/core/fbs/model",
"src/core/fbs/fbs.ts",
"src/core/publizon/model",
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Nice. I agree that it does not make sense to lint our generated code.

}
},

settings: {
react: {
version: "16.11.0"
version: "detect" // Automatically detect the React version
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 This is better than using a fixed version.

package.json Outdated
Comment on lines 56 to 60
"@cypress/webpack-preprocessor": "^6.0.2",
"@eslint/eslintrc": "^3.2.0",
"@eslint/js": "^9.17.0",
"@cypress/browserify-preprocessor": "^3.0.2",
"@cypress/code-coverage": "^3.13.10",
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider reordering these dependencies in alphabetical order.

A I understand it the dependencies are normally in alphabetical order. I wonder if this is the result of a merge conflict. If we leave it as is we risk pushing an unwanted change to the next developer.

If this alphabetical ordering is really a rule we could also consider adding a CI check to ensure that it is kept consistent 🤔

Comment on lines +17 to 18
// eslint-disable-next-line no-console
console.log("Replacement results:", results);
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider allowing console.log within /scripts.

It seems reasonable that we use console in these cases. On the other hand I think that we may be removing this post processing going forward so perhaps it does not make sense to do too much here.

Comment on lines +24 to +25
// eslint-disable-next-line react/display-name
({ ...props }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned previously.

@@ -7,6 +7,7 @@ export default function withSuffix<T extends object>(
suffix: string,
reduxAction: ActionCreatorWithPayload<unknown, string>
) {
// eslint-disable-next-line react/display-name
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned previously.

Comment on lines 81 to 84
it("should handle erroneous requests", async () => {
const { result } = renderHook(() =>
useMultipleRequestsWithStatus({
requests: [
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Nice that we can get this test back!

src/components/alert/alert.jsx Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@kasperg kasperg assigned ThomasGross and unassigned kasperg Jan 16, 2025
@ThomasGross ThomasGross requested a review from kasperg January 22, 2025 10:14
package.json Outdated
Comment on lines 105 to 107
"eslint-config-airbnb": "^19.0.4",
"eslint-config-airbnb-typescript": "^18.0.0",
"eslint-config-prettier": "^10.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please consider this change.

As far as can tell this adds packages that we actually want to remove.

src/core/utils/modal.tsx Show resolved Hide resolved
@ThomasGross
Copy link
Contributor Author

ThomasGross commented Jan 24, 2025

👍 Appreciate all your updates into this. We have gone far but there is still a bit of way.

Three important things worth highlighting:

  1. We need to update the documentation regarding code guidelines. This has explicit references to Airbnb. I am considering whether this even needs an ADR describing the (updated) choice of coding standards.
  2. We have an recurring issue about react/display-name that we need to figure out what to do about. It seems to be specifically related to our use of Higher-Order Components. To me the reasoning behind the rule makes sense: "This name is used by React in debugging messages". If we do not appreciate this - or there is generally is no quick fix for this - then we should consider disabling the rule altogether.
  3. The situation about useIntersection() seems to be unresolved. Let's talk about that.

I have created new jira tasks on each of these topics.
https://reload.atlassian.net/browse/DDFBRA-405
https://reload.atlassian.net/browse/DDFBRA-406
https://reload.atlassian.net/browse/DDFBRA-407

@ThomasGross ThomasGross merged commit ced8c07 into develop Jan 24, 2025
19 of 20 checks passed
@ThomasGross ThomasGross deleted the react-19-update branch January 24, 2025 11:55
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