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

Add support for React 18 #3203

Open
wants to merge 1 commit into
base: v4
Choose a base branch
from
Open

Add support for React 18 #3203

wants to merge 1 commit into from

Conversation

jonkoops
Copy link
Contributor

Adds support for React 18 and applies the migrations as stated in the migration guide. This change is part of a greater effort to land React 18 support in Patternfly (patternfly/patternfly-react#7142).

const renderFn = isProd ? ReactDOM.hydrate : ReactDOM.render;
renderFn(<App />, document.getElementById('root'));
const container = document.getElementById('root');
// TODO: Enable StrictMode: https://reactjs.org/docs/strict-mode.html
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enabling strict mode here seems to break the navigation, so I have omitted it in this PR. I've created an issue to deal with this at a later point (#3204).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah since PF is built with so many PF components, and there are a number of issues with strict mode in PF-react right now, it may be a time before we can enable strict mode in org.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicolethoen note that this is React Strict Mode, not TypeScript strict mode we're talking about.

@patternfly-build
Copy link
Contributor

patternfly-build commented Sep 21, 2022

@jschuler
Copy link
Collaborator

LGTM
I believe the release process uses lerna with conventional commits, how about we make this a major release for documentation-framework package?
I think you can do that by starting the commit message with BREAKING CHANGE

@nicolethoen
Copy link
Collaborator

@jonkoops once you rebase and update the commit message, we can merge this

@jonkoops
Copy link
Contributor Author

jonkoops commented Jan 8, 2023

Done 👍

@evwilkin
Copy link
Member

@dgutride @wise-king-sullyman @dlabaj I wanted to get your eyes on this as the docs-framework is no longer used just for the PF website docs but is the basis for all PF extension docs moving forward. Based on the potential impact to extensions do you have feedback on this PR to support React 18?

@wise-king-sullyman
Copy link
Collaborator

I did a quick test in the extracted catalog view repo by pulling this branch and using yarn link, and trying to start the docs framework development server gives the following error and fails to compile.

[0] ERROR in /Users/ausulliv/repos/patternfly-org/packages/documentation-framework/app.js
[0] Module not found: Error: Can't resolve 'react-dom/client' in '/Users/ausulliv/repos/patternfly-org/packages/documentation-framework'

@jonkoops do you have any guesses as to the issue here? What I've found on Stack Overflow so far suggests that the issue is usually caused by using a React version < 18, but the extracted catalog view is already using React 18.

@nicolethoen
Copy link
Collaborator

I'm also concerned that if this presents a breaking change for patternfly-react to consume the documentation framework, that we'd only be able to consume this doc-framework update in the v5 branch and will mean we cannot consume any updates to the documentation framework in v4 going forward. that sounds like it might be a problem. I'll check out trying to use this updated version in patternfly-react's v4 branch and see what happens

@nicolethoen
Copy link
Collaborator

We might need to wait to merge this until we are able to create a PFv4 branch the documentation framework. I'll loop back around when the decisions about v4 are made

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.

6 participants