-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 18 #5359
base: main
Are you sure you want to change the base?
Bump react to 18 #5359
Conversation
|
Differences Found✅ No packages or licenses were added. SummaryExpand
|
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.
I think we should:
- get rid of
ts-ignore
in favor ofts-expect-error
+ add description why it's needed? - don't use
import * as React from "react";
since we're importing only selected functions in rest of our codebase - replace
ReactChildren
type withReactNode
if it makes sense - I don't think we should limit what children should be passed if we're not explicitly requiring them to be React components - replace macaw with stable version
- change that ESLint config as they suggested in docs
@@ -91,6 +91,7 @@ | |||
], | |||
// Decided to turn off: | |||
"react/prop-types": "off", | |||
"react/react-in-jsx-scope": "off", |
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.
suggestion: according to react eslint plugin docs, we should instead extend react/jsx-runtime
config:
If you are using the new JSX transform from React 17, you should disable this rule by extending react/jsx-runtime in your eslint config (add "plugin:react/jsx-runtime" to "extends").
@@ -30,7 +30,7 @@ | |||
"@material-ui/styles": "^4.11.4", | |||
"@reach/auto-id": "^0.16.0", | |||
"@saleor/macaw-ui": "npm:@saleor/[email protected]", | |||
"@saleor/macaw-ui-next": "npm:@saleor/macaw-ui@1.1.15", | |||
"@saleor/macaw-ui-next": "npm:@saleor/macaw-ui@0.0.0-pr-20250127234423", |
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.
issue: is this a mistake? if we have unreleased changes we should probably release new macaw version
{/* eslint-disable-next-line @typescript-eslint/ban-ts-comment */} | ||
{/* @ts-ignore */} |
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.
suggestion: we should use @ts-expect-error
instead of @ts-ignore
, because if for some reason we don't have TypeScript error anymore it will be shown as error so that we can remove this comment. You also don't need to disable ESLint rule for that (probably)
{/* eslint-disable-next-line @typescript-eslint/ban-ts-comment */} | ||
{/* @ts-ignore */} |
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.
suggestion: same here let's use @ts-expect-error
// eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
// @ts-ignore |
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.
@ts-expect-error
@@ -6,7 +6,7 @@ import { getFormErrors } from "@dashboard/utils/errors"; | |||
import { TextField } from "@material-ui/core"; | |||
import { makeStyles } from "@saleor/macaw-ui"; | |||
import { Option } from "@saleor/macaw-ui-next"; | |||
import React from "react"; | |||
import * as React from "react"; |
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.
question: why do we need this import, can we get rid of it?
Scope of the change