Detect Fiber types to avoid inlining constants #1790
Conversation
| function detectFiberTags() { | ||
| const supportsMode = typeof React.StrictMode !== 'undefined'; | ||
| const supportsContext = typeof React.createContext !== 'undefined'; | ||
| const supportsForwardRef = typeof React.forwardRef !== 'undefined'; |
There was a problem hiding this comment.
These assume you're not going to use a newer react minor with an older react-dom minor. If that's not an assumption we're comfortable making, I can add some try-catches and fall back.
There was a problem hiding this comment.
I think that it's fine if things break with a mismatched react/react-dom - those have always had to match minor versions to work properly.
There was a problem hiding this comment.
Thanks - as discussed, my understand is that this is needed because:
- the internal "tag" values are constants inside react-dom
- react-reconciler is hardcoded to those values, which makes its compatibility with react-dom both brittle and quasi-accidental
- react-dom v16.4.3 will change these internal values, for perf reasons and as part of a larger internal refactor.
- It can't be part of a v16.5 (which would make many things easier) simply because there's nothing new to release that would make it minor, but the react team needs this internal refactor to be published now.
- v1 of the 16 adapter needs to retain back compat with 16.0 - 16.4.3, which means it can't just change to the updated hardcoded values (while this would be an option in a v2 of the 16 adapter, that would leave us in the same potential breaks-in-a-patch situation as we are now)
I'll merge this once tests pass and cut the appropriate releases.
|
Above sounds correct. What do you think about my concern re: mismatching React and ReactDOM versions? Do you aim to support this? |
|
I replied:
Although it's ideal if react and react-dom only need to match within minors, i think it's reasonable to require that they match exactly. I think that could be helped, however, by making react-dom 16.4.3 peerDepend on |
|
Sorry, GH didn't show me your comment at first.
FWIW the only case I anticipate breaking is when |
|
In the past, I've seen many bugs caused by mismatched minors between react and react-dom; I think it is both an actual problem in practice, but also one that's easily addressed by telling people to manually keep those versions in sync. If enzyme helps surface that sooner than production, that's a feature not a bug imo :-D |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Thanks for doing this Dan! ❤️ |
- [New] Add forwardRef support (#1592, @jquense) - [New] Add Portal support (#1760, #1761, #1772, #1774, @jgzuke) - [New] Add pointer events support (#1753, @ljharb) - [New] Add `displayNameOfNode`, `isValidElementType` (#1701, @jquense) - [New] pass the adapter into `createMountWrapper` (#1592, @jquense) - [Fix] preemptively fix compat with React v16.4.3 (#1790, #1778, @gaearon, @aweary) - [Fix] `shallow`: skip updates when nextState is `null` or `undefined` (#1785, @koba04) - update deps - [meta] ensure a license and readme is present in all packages when published
- [New] Add forwardRef support (#1592, @jquense) - [New] Add Portal support (#1760, #1761, #1772, #1774, @jgzuke) - [New] Add pointer events support (#1753, @ljharb) - [Fix] preemptively fix compat with React v16.4.3 (#1790, #1778, @gaearon, @aweary) - [Fix] `shallow`: prevent rerenders with PureComponents (#1786, @koba04) - [Fix] `shallow`: skip updates when nextState is `null` or `undefined` (#1785, @koba04) - [Fix] `shallow`: `setState` after `setProps` calls `componentWillReceiveProps` (#1779, @peanutenthusiast) - [Fix] `mount`/`shallow`: be stricter on the wrapper’s setState/setProps callback - [Fix] `shallow`/`mount`: improve error message when wrapping invalid elements (#1759, @jgzuke) - update deps - [Refactor] remove most uses of lodash - [meta] ensure a license and readme is present in all packages when published
This removes Enzyme's reliance on internal React reconciler constants which can change between releases (and are going to change in 16.4.3 which we plan to cut on Monday).
Instead, we're feature testing to detect the Fiber tags. The fields I'm relying on are relatively stable, and are expected to stay significantly more stable than the constants. The detection is done lazily (because we can't assume the DOM is available until you call
mount). I've checked that other code paths don't useFiberTags.This should work with all 16.x versions. I did some manual checking with 16.1, 16.2, 16.3, and the master React version (which would break without this). I haven't verified 16.0 but I'd expect it to work as well.
I included the revert of the reconciler dependency. This is because the code in it does rely on the constants (which we don't want). The inlined version here doesn't (because those constants are used only for invariants).
Longer term, we should be able to get rid both of this inlined piece, and of the constant detection. Instead we should share the infrastructure with React DevTools. I expect that we'll get to it in the coming months, and when we do, I'll send a PR for that. In the meantime I'm happy to help with bugfixing the React adapter if you discover any problems with my approach.