React 18 support

[sybernatus]

Update to support react 18.
Related issue : #8126

Description

• Upgrade to react 18.2.0
• Upgrade to react-dom 18.2.0
• Upgrade to react-test-renderer 18.2.0
• Change the @wojtekmaj/enzyme-adapter-react-17 library to @cfaester/enzyme-adapter-react-18
• Replace ReactDOM usage (deprecated in react 18) by createRoot

Motivation and Context

The change is required to integrate swagger-ui to react 18 application

How Has This Been Tested?

• By running the build pipelines using npm scripts
• By serving the application locally
• By running test pipelines (npm run test & npm run test-in-node)

Screenshots (if appropriate):

Checklist

My PR contains...

• No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
• Dependency changes (any modification to dependencies in package.json)
• Bug fixes (non-breaking change which fixes an issue)
• Improvements (misc. changes to existing features)
• Features (non-breaking change which adds functionality)

My changes...

• are breaking changes to a public API (config options, System API, major UI change, etc).
• are breaking changes to a private API (Redux, component props, utility functions, etc.).
• are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
• are not breaking changes.

Documentation

• My changes do not require a change to the project documentation.
• My changes require a change to the project documentation.
• If yes to above: I have updated the documentation accordingly.

Automated tests

• My changes can not or do not need to be tested.
• My changes can and should be tested by unit and/or integration tests.
• If yes to above: I have added tests to cover my changes.
• If yes to above: I have taken care to cover edge cases in my tests.
• All new and existing tests passed.

[char0n]

Hi @sybernatus,

Thanks for the work on this. I was wondering is you see any warnings or errors in console.

This is candidate for inclusion into v5 release, from the next branch.

[sybernatus]

Hi @char0n,

I ran the app on my local and no errors/warnings showed up in the console.

Do I need to perform another action? 🚀

[char0n]

@sybernatus I'm currently in the process of consolidation next branch. When done, I'll ping you and will ask to you to send the PR against the next branch.

[char0n]

@sybernatus would you mind sending this PR against the next branch?

[sybernatus]

Done. Tell me if you need another action on my side 🙂

[char0n]

@sybernatus thanks!

Let me explain what are the next steps and expectations:

We have to introduce, React@18 in completely backward compatible way. This means that we'll have to support following range in package.json

"react": ">=17.0.2 <19",
"react-dom": ">=17.0.2 < 19",

If upstream consumer will need to pin the react version, it would be possible in npm with overrides field and in yarn with resolutions field.

---

Next thing is render method, which is plugged in SwaggerUI plugin system via rootInjects.
We can amend the render method and use new ReactDOM api depending on React.version field - allowing to kick in the react@18 concurrent renderer or using the react@17 fallback mode.

---

Of course our tests will assume react@18 is always installed (to eliminate complexity).

[sybernatus]

Do you mean something like that ?

[char0n]

@sybernatus,

Yes something like that, but more scalable. I'd modify the render to look like this:

export const render = (getSystem, getStore, getComponent, getComponents) => (domNode) => {
  const App = getComponent(getSystem, getStore, getComponents)("App", "root")
  const { createRoot } = require('react-dom/client')
  const root = createRoot(domNode)
  root.render(<App/>)
}

Reasoning: this is forward compatible way of mounting react now. It will probably not change for some time now. We use require because we need the code to be called in runtime and not build-time. Webpack@5 will handle this require well.

Next I'd introduce react-17 plugin in src/core/plugins:

src/core/plugins/react-17/root-injects.js

 export const render = (getSystem, getStore, getComponent, getComponents) => (domNode) => {
  const App = getComponent(getSystem, getStore, getComponents)("App", "root")
  const ReactDOM = require("react-dom")
  ReactDOM.render(<App/>, domNode)
}

src/core/plugins/react-17/index.js;

import { render } from "./root-injects"

const React17Plugin = ({ getSystem }) => {
   const { React } = getSystem()
   const rootInjects = {}

   if (React.version?.match(/^17/)) {
     rootInjects.render = render;
   }

   return {
     rootInjects,
   }
}

Now this would basically override the render function for React@17 and in future, when React@17 is EOL, we'll just delete the react-17 plugin. If you want to proceed with this, please continue with this spirit + don't forget to amend the react/react-dom ranges in packagej.json.

[sybernatus]

Thanks, it's done 🙂

[char0n]

We have number of styling issues

[sybernatus]

I'm checking sorry, tests are not passing.

[sybernatus]

I have one test that I can figure out why it's not passing.
Can you help me to solve it please?
file: test/unit/core/utils.js line 1493

  ● utils › buildUrl › throws error when server url contains non-transcluded server variables

    expect(received).toThrow(expected)

    Expected pattern: /^Invalid base URL/
    Received message: "Invalid port"

I didn't change any inputs, I think so I don't understand why the received message changed to Invalid Port

[char0n]

Hi @sybernatus,

I have one test that I can figure out why it's not passing.

The jest tests are depending on jsdom which uses whatwg-url package under the hood. When jsdom was last updated I've aligned the new error message. More details in #8512

Now in this case, it looks like from what ever reason the older version of jsdom was installed. Feel free to mark the test as skipped for now.

[char0n]

Hi again @sybernatus,

I had a hour to spare today and followed up on your work in #8888. I've incorporated your changes + some additional changes that were required for backward compatibility with React@17 and React@16.

Now the question is how we're going to do the merge so that we don't loose your attribution. I'm suggesting you merge char0n/react-18-support into your sybernatus:ft/react-18-support branch. As soon as the merge is done and pushed, I'll do some last changes (regenerate package-lock.json) and I'll merge your PR. Would that work for you?

[char0n]

Hm, I can actually see regressions in SwaggerUI behavior on React@18. One of these regressions can be seen here: https://github.com/swagger-api/swagger-ui/actions/runs/5190826908/jobs/9357817486?pr=8888. This needs to be investigated before we can go further.

[sybernatus]

You're right I have also this cypress issues in local. I'm checking.

[char0n]

You're right I have also this cypress issues in local. I'm checking.

I did spent couple of minutes on this regression, but couldn't find a root cause. We'll have to inspect the components involved and find out why this happens. I'll try to find time during this week to look at it in more details.

[sybernatus]

Indeed, I spent several hours yesterday tackling this issue. Since I'm new to this repository, I'm still figuring out where the loading process for the URL test page is located. Regardless, this is a definite regression, as I was able to reproduce the issue locally.

I'll find some more time this week on this.

[char0n]

@sybernatus I've update the Cypress to latest version so that we have the latest testing tools. Please rebase your branch on top of next branch. We'll have to resolve the conflicts.

[sybernatus]

It seems that going from React 17 to 18, the re-rendering of server.jsx is not well executed.
Even with the props correctly updated, the component for basePath is not re-render when selecting another value in the top-right dropdown.

the function getServerVariable() passed to the component always return the first rendered value, like memoized or something similar.

If you have any idea how to solve this, I take it. 😵‍💫

Still trying to figure it out

---

EDIT

It seems that the function UNSAFE_componentWillReceiveProps() in server.jsx is triggered multiple times in react 17 but only once in React 18.

In react 17

• 1st trigger UNSAFE_componentWillReceiveProps() => conditions in the functions are false so does nothing
• 2nd trigger of UNSAFE_componentWillReceiveProps() => conditions are true so it changes the basePath of the component
  In react 18
• There only 1 trigger of UNSAFE_componentWillReceiveProps() => conditions in the functions are false so does nothing

Still investigating why..

[char0n]

@sybernatus any luck so far?

And thank you for pinpointing the actual place of issue. I assume that UNSAFE_componentWillReceiveProps might be problematic in all places where used.

[char0n]

Some more additional info https://www.geeksforgeeks.org/reactjs-unsafe_componentwillreceiveprops-method/

[char0n]

This PR has been superseded by #9435.