dequelabs / cauldron

https://cauldron.dequelabs.com/
Mozilla Public License 2.0
92 stars 21 forks source link

Upgrade to React v18 #631

Closed stephenmathieson closed 1 week ago

stephenmathieson commented 2 years ago

Once #1159 and #1473 is complete, we should upgrade to the latest version of React 18. This should include both react and react-dom for development dependencies in the following packages:

Peer dependencies can be changed to the following:

"peerDependencies": {
  "react": ">=16.6",
  "react-dom": ">=16.6"
}

Also see https://react.dev/blog/2022/03/08/react-18-upgrade-guide for a reference of changes that may be needed. Running the newly updated tests should help catch any lingering issues, but a full breadth testing of Cauldron documentation should be done to identify any possible issues with each component.

scurker commented 2 years ago

There's a possibility that we could unofficially support React 17/18, I'm not seeing any breaking changes for 17. We could temporarily use this third party adapter to allow cauldron to use React 17. There may not be anything preventing someone from using cauldron in 18, we would just need to validate within the apps.

scurker commented 2 years ago

It looks like there's some breaking changes with v18:

stephenmathieson commented 2 years ago

The updates to the types are what's preventing us from upgrading axe.deque.com to React 18. We should get those sorted out ASAP.

abbeyperini-deque commented 2 years ago

The modals break when you upgrade to react-dom 17. Modal toggle is called twice on open in 4.7.0-canary.a816b304 (fix from Pr #698) and four times with 4.7.0-canary.87de5e62. It appears that the onClose event is always called by clickOutsideListener probably because of changes to event delegation especially with document.addEventListener.

OptionsMenu also will not open.

scurker commented 2 years ago

The modals break when you upgrade to react-dom 17. Modal toggle is called twice on open in 4.7.0-canary.a816b304 (fix from Pr #698) and four times with 4.7.0-canary.87de5e62. It appears that the onClose event is always called by clickOutsideListener probably because of changes to event delegation especially with document.addEventListener.

@schne324 and I noticed this yesterday on React@18, but I don't think it's necessarily directly related to the event delegation changes.

ClickOutsideListener does not use onClick handlers but manages its own events:

https://github.com/dequelabs/cauldron/blob/a816b304ac4667e3c3734d4147a044fcf975961f/packages/react/src/components/ClickOutsideListener/index.tsx#L70-L76

The issue I noticed is that addEventListener for ClickOutsideListener was somehow getting mounted before the triggering element causing the onClickOutside to be invoked with the triggering element. My guess between 16/18 something has changed with scheduling. It's possible that automatic batching could be the cause for react@18, but we need to investigate this further.

abbeyperini-deque commented 2 years ago

🤔 e.stopPropagation() in the onClick in the modal before the modal toggle call prevented onClose from being called and now the modals work correctly.

Decided to swap to TopBarMenu and OptionsMenuList instead of debugging OptionsMenu.

Suvendhu98 commented 1 week ago

Hi @scurker any qa notes for the ticket?

scurker commented 1 week ago

Hi @Suvendhu98 do you normally test Cauldron components? I already did a small regression sweep of components and everything checked out. If you would like to do some exploratory testing you can.

Suvendhu98 commented 3 days ago

We don't test cauldron specifically , but we do ui/ux testing during regression , as of now we didn't find any issues over there , everything looks good , hence closing the ticket