-
Notifications
You must be signed in to change notification settings - Fork 377
chore(deps): Update components to use new JSX transform #11582
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
Conversation
|
Preview: https://patternfly-react-pr-11582.surge.sh A11y report: https://patternfly-react-pr-11582-a11y.surge.sh |
4a81f9d to
d193708
Compare
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.
Because the tests are being excluded in the TSConfig the new "jsx": "react-jsx" option is not being applied to them. This means that when you open a test file that has JSX in it (e.g. Chart.test.tsx) you'll get an error message.
This will have to be fixed so that either the tests are no longer excluded from tsconfig.base.json or another TSConfig should be added for the tests that extends this base and then includes the tests again. Additionally, this means the tests are currently not being type-checked, which can lead to accidental regressions.
Note that the same currently applies to the example projects, which seem to have actual type errors. Perhaps this is best handled separately in a different PR so this one can be rebased on top.
ab4fcb7 to
e0ac221
Compare
The troublesome part of this is that I don't see any these errors, either in the build or in the IDE, although I know you are experiencing them. It's making it difficult to debug and find a solution when I'm not able to reproduce on my end. I'll keep at it, perhaps we need a tests-specific tsconfig that has the same JSX options as the base tsconfig. |
|
I took a look at all the files, and it all looks good to me. As far as I am concerned this PR can be merged, and I think we should do it sooner rather than later to avoid conflicts. |
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.
As discussed on Slack I don't love that some of the markdown examples now aren't including the react related imports directly in the examples themselves.
Other than that LGTM, and as discussed I'm fine with that fix being in a followup PR so that this can get merged sooner rather than later.
React is going to a place where the compiler is responsible for making the right imports, so users should really be instructed to no longer include it. I'd argue it is good to educate them now, if you bootstrap a new React project this behavior is the default. |
Hook imports and such will no longer be needed? |
Oh sorry, I thought you were referring to the |
tlabaj
left a comment
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.
LGTM
dlabaj
left a comment
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.
LGTM
thatblindgeye
left a comment
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.
As part of the rebase, we shoulld revert the default value of the forwardScrollAriaLabel prop in NavList and instead open an issue to fix that in the next breaking change. Ohterwise components and demos looked good.
e0ac221 to
3ad9e07
Compare
dd88df8 to
50e0183
Compare
What: Closes #11553
This addresses the new JSX transform changes that were first introduced in React 17, in short, we no longer require import react statements for files that only needed it for JSX. Files that require a react import for non-globals e.g. for hooks like
useEffectwill still require the import, but they must be named imports instead of the genericimport React from 'react',import * as React from 'react', etc.I've updated the associated eslint rule to throw an error now that the sweep is complete and everything is building cleanly.
Including @jonkoops on this as well.