microsoft / fluentui

Fluent UI web represents a collection of utilities, React components, and web components for building web applications.
https://react.fluentui.dev
Other
18.33k stars 2.72k forks source link

Upgrade our Repo to React 17, tests, VR, examples, etc. #20145

Closed ling1726 closed 2 years ago

ling1726 commented 2 years ago

[some partners are rolling out R17 now, others are planning]

ling1726 commented 2 years ago

Enzyme does not officially support React 17 enzymejs/enzyme#2430

An unofficial adapter exists, but no guarantee all tests work https://www.npmjs.com/package/@wojtekmaj/enzyme-adapter-react-17

ecraig12345 commented 2 years ago

Related: #15732, #16886

ecraig12345 commented 2 years ago

After #22265, all the tests included in yarn test appear to be working with React 17 and @wojtekmaj/enzyme-adapter-react-17.

ecraig12345 commented 2 years ago

Trying out React 17 again (draft): #22326

Probably everything works except a weird issue with unmounting northstar Popup in tests (not sure if it's a real issue too): https://github.com/microsoft/fluentui/pull/22326#pullrequestreview-934343344

ecraig12345 commented 2 years ago

As of writing, all the tests work against React 17 (it's possible new breaks could be introduced), but the upgrade attempt (#22326) got blocked when I tried to update the @types/react dev dep to 17.

The latest @types/react 17 version as of writing (17.0.44) includes a reference to Iterable, which is from lib.es2015 and therefore won't compile in v8 packages that only reference lib.es5.

type ReactFragment = {} | Iterable<ReactNode>;

Iterable usage was introduced in 17.0.32, so you could probably work around the issue by setting the repo's @types/react dev dep to 17.0.31. (Anyone looking back at this issue later can also check latest 17 to see if the issue has been resolved.)

Some clarifying notes:

* Qualifiers for "should not": The repo's internal @types/react version hopefully shouldn't affect consumers, but I can see a couple ways this could break (so from this perspective, staying on 16 dev dep might be safer).

micahgodbolt commented 2 years ago

Sean mentioned that event delegation in layers in React 17 can cause some issues. Make sure to take a look at that area.