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

Detect Fiber types to avoid inlining constants #1790

Merged
merged 2 commits into from
Aug 24, 2018

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Aug 24, 2018

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 use FiberTags.

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.

function detectFiberTags() {
const supportsMode = typeof React.StrictMode !== 'undefined';
const supportsContext = typeof React.createContext !== 'undefined';
const supportsForwardRef = typeof React.forwardRef !== 'undefined';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Brandon Dail and others added 2 commits August 24, 2018 10:34
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks - as discussed, my understand is that this is needed because:

  1. the internal "tag" values are constants inside react-dom
  2. react-reconciler is hardcoded to those values, which makes its compatibility with react-dom both brittle and quasi-accidental
  3. react-dom v16.4.3 will change these internal values, for perf reasons and as part of a larger internal refactor.
  4. 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.
  5. 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.

@gaearon
Copy link
Contributor Author

gaearon commented Aug 24, 2018

Above sounds correct.

What do you think about my concern re: mismatching React and ReactDOM versions? Do you aim to support this?

@ljharb
Copy link
Member

ljharb commented Aug 24, 2018

I replied:

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.

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 ^16.4.3 of react - that would solve everything except "react 16.4.3, older react-dom", but i'm not sure there's a solution for that.

@gaearon
Copy link
Contributor Author

gaearon commented Aug 24, 2018

Sorry, GH didn't show me your comment at first.

I think that could be helped, however, by making react-dom 16.4.3 peerDepend on ^16.4.3 of react - that would solve everything except "react 16.4.3, older react-dom", but i'm not sure there's a solution for that.

FWIW the only case I anticipate breaking is when react has a larger minor than react-dom (e.g. react@16.4 with react-dom@16.3). Because our feature checks would pass but then the detection would fail. Let's see if this turns out to be a problem in practice.

@ljharb
Copy link
Member

ljharb commented Aug 24, 2018

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

@gaearon

This comment has been minimized.

@ljharb

This comment has been minimized.

@ljharb ljharb merged commit 003bc36 into enzymejs:master Aug 24, 2018
@aweary
Copy link
Collaborator

aweary commented Aug 24, 2018

Thanks for doing this Dan! ❤️

ljharb added a commit that referenced this pull request Aug 25, 2018
 - [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
ljharb added a commit that referenced this pull request Aug 25, 2018
 - [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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants