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

Forward ref #1592

Merged
merged 6 commits into from
Aug 16, 2018
Merged

Forward ref #1592

merged 6 commits into from
Aug 16, 2018

Conversation

jquense
Copy link
Contributor

@jquense jquense commented Mar 23, 2018

This is built on top the Context PR, i'll rebase once that is merged

Adds support for forwardRef to accompany the new changes.

The dependencies for these to work are getting fairly specific at this point, I’d like to bump the dep range for react-test-renderer, but that effectively bumps the peer deps for react, maybe worth splitting to a new adapter at this point?

It would be really great if we could use toTree from react-test-renderer directly…

Fixes #1604.

@petegleeson
Copy link

Hi mate can we rebase these changes with master now #1513 is merged?

The lack of forwardRef support is a bit of a blocker for us at the moment. I have time to work on this so please let me know if there is anything I can do.

@jquense
Copy link
Contributor Author

jquense commented Apr 4, 2018

CI failure looks like a travis stalled build

Copy link

@petegleeson petegleeson left a comment

Choose a reason for hiding this comment

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

I have verified these changes fix our use-case for forwardRef. 👍

As you mention, I found the versions to be pretty fiddly. In our codebase, I got runtime errors after running yarn add enzyme enzyme-adapter-react-16. This was due to dependencies like react-test-renderer and enzyme-adapter-utils not being bumped to the latest. Running yarn upgrade enzyme enzyme-adapter-react-16 solved this problem for me.

export const ITERATOR_SYMBOL = typeof Symbol === 'function' && Symbol.iterator;

// TODO use react-is when a version is released with no peer dependency on React

Choose a reason for hiding this comment

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

The latest react-is release has your change in it and does not have react as peer dependency anymore. Could this be updated?

Copy link
Member

Choose a reason for hiding this comment

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

a) react-is must never be used in new places outside of a react adapter, since non-react adapters exist. b) react-is does not work on react 0.13.

So, all of these things need to be exposed via the adapter interface - instead of being part of enzyme.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what's actionable here. It' seems like you are saying we should update all adapters to provide a typeOf api?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, i think that might be what's required :-/

if so, let's do that in a separate PR, so that when this one is rebased, it can only include the diffs to typeOf?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oof ok yeah i'll pull this out for now

@azmenak
Copy link

azmenak commented Apr 23, 2018

As @petegleeson mentioned, this PR seems to be a blocker for anyone using forwardRefs. Anything we can do to get this moving?

Seems the react-is changes are strait forward, is there any way to add those changes to this PR? Happy to make those edits if it helps.

@sambokai
Copy link

sambokai commented May 20, 2018

Seems like this is solved by: facebook/react#12725

@janv
Copy link

janv commented Jun 15, 2018

What's holding this back?

@cyan33
Copy link

cyan33 commented Jun 18, 2018

Does this PR fix the forwardRef issue and is good to merge? Much appreciated if this gets merged soon. 😄

@nemoDreamer
Copy link

Yup... I'm about to switch my tests that rely on this back to Facebook's TestRenderer... 😢

@jquense
Copy link
Contributor Author

jquense commented Jun 27, 2018

If anyone needs an interim solution i've released @monastic.panic/enzyme-adapter-react-16 folks can use

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 - this needs a number of changes before it can land.

@@ -55,7 +60,7 @@ export default function createMountWrapper(node, options = {}) {
}
}
WrapperComponent.propTypes = {
Component: PropTypes.oneOfType([PropTypes.func, PropTypes.string]).isRequired,
Component: PropTypes.oneOfType([PropTypes.func, PropTypes.string, specialType]).isRequired,
Copy link
Member

Choose a reason for hiding this comment

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

PropTypes.element?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its not an element right it's a component class, e..g element.type

Copy link
Member

Choose a reason for hiding this comment

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

$$typeof isn't in react 13 (which is why react-is doesn't work on it), so i'm not sure that the propType is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

o I see what you mean, PropTypes.element won't fire validate a Context.Consumer or Forward ref "class" or type correctly unfortunately. I guess we could switch on react version for the proptype and not include teh specialType below 16?

Copy link
Member

Choose a reason for hiding this comment

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

We could have multiple things here - but we might want to add a propType to each adapter for "a component" - that way react 0.13 can specify that the function has to be a class component, for example, and not an SFC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isValidElementType that we added would be the the right check I think. We could pass that in and use it in a custom proptype

Copy link

Choose a reason for hiding this comment

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

@jquense This may be helpful since I just figured this same prop type check out: facebook/prop-types#200 (comment)

(Eagerly awaiting this PR merge)

@@ -37,9 +38,16 @@ if (is('^16.3.0-0')) {
createContext = null;
}

if (is('^16.3.0-0')) {
({ forwardRef } = require('react'));
Copy link
Member

Choose a reason for hiding this comment

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

let's combine createContext with forwardRef, since they're in the same versions.

@@ -66,6 +94,7 @@ function indentChildren(childrenStrs, indentLength) {

export function debugNode(node, indentLength = 2, options = {}) {
if (typeof node === 'string' || typeof node === 'number') return escape(node);
if (typeof node === 'function') return '[function child]';
Copy link
Member

Choose a reason for hiding this comment

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

why is this necessarily a function child?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what else to call a props.children that's a function. Perhaps cases also hit here, but i'm not sure what they are.

Copy link
Member

Choose a reason for hiding this comment

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

right but debugNode won't necessarily be called on a child.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair, should we just call it [function] then?

export const ITERATOR_SYMBOL = typeof Symbol === 'function' && Symbol.iterator;

// TODO use react-is when a version is released with no peer dependency on React
Copy link
Member

Choose a reason for hiding this comment

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

a) react-is must never be used in new places outside of a react adapter, since non-react adapters exist. b) react-is does not work on react 0.13.

So, all of these things need to be exposed via the adapter interface - instead of being part of enzyme.

export const Portal = hasSymbol ? Symbol.for('react.portal') : 0xeaca;
export const StrictMode = hasSymbol ? Symbol.for('react.strict_mode') : 0xeacc;

export function typeOf(node) {
Copy link
Member

Choose a reason for hiding this comment

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

this needs unit tests.

case ContextProvider:
case ContextConsumer:
case Mode:
case ForwardRef:
Copy link

Choose a reason for hiding this comment

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

If we want to find the ForwardRef itself, we should add a node for it, not only its children here in this case.

@cyan33
Copy link

cyan33 commented Jun 29, 2018

To fix finding forwardRef itself, I had to change this function:

export function nodeMatchesObjectProps(node, props) {
  if (typeOf(node) === ForwardRef) {
    return isSubset(node.type, replaceUndefinedValues(props));
  }
  return isSubset(propsOfNode(node), replaceUndefinedValues(props));
}

and in the react-16-adapter, in the toTree method:

case ForwardRef:
    return {
        nodeType: 'forward_ref',
        type: node.type,
        props: { ...node.memoizedProps },
        rendered: childrenToTree(node.child),
     };

Not sure if it's decent but it fixes my issue.

@jquense
Copy link
Contributor Author

jquense commented Jun 30, 2018

Alright, as @cyan33 notes, the problem here is that we can't add support for ForwardRef selecting without some sort of brand check.

mount(<TreeWithForwardRef/>).find(ForwardRefComponent)

Doesn't work here because selectors are pretending to be agnostic, but are actually codifying the assumption that element types can only be functions or strings, which isn't true anymore for React. The simplest but dirty approach is to go with what I have here for typeof logic, which is honestly an extension of the current state of affairs, even if that SOA isn't the intended one. The alternative, as @ljharb notes, is to add a isValidElementType api to adapters and pass that through to the selectors, which is a much bigger refactor.

@ljharb
Copy link
Member

ljharb commented Jun 30, 2018

I think adding that API to the adapters is a fine idea; let's just do it in a separate PR and rebase this after it's merged.

@wmonk
Copy link
Contributor

wmonk commented Jul 20, 2018

Thanks for your work on this! Is there any capacity to get this moving forward? I've tried the patched packages but none of them seemed to work.

@wmonk
Copy link
Contributor

wmonk commented Aug 16, 2018

🎉 is there an ETA for this getting published to npm?

@ljharb
Copy link
Member

ljharb commented Aug 16, 2018

It was merged 9 hours ago; you asked 6 hours ago. Have some patience.

@jquense
Copy link
Contributor Author

jquense commented Aug 16, 2018

Thanks for finishing this up @ljharb!

@quantizor
Copy link

@ljharb We all want our golden goose and want it now! 😂

@quantizor
Copy link

quantizor commented Aug 20, 2018

@ljharb looks like there's been a few enzyme patch releases since this was merged. Could we get a react 16 adapter release also to start using this? 🙇

@ljharb
Copy link
Member

ljharb commented Aug 20, 2018

@probablyup those were backports that don't include this (or other semver-minor) changes. This feature remains unreleased, but will be released when things are ready.

@kenips
Copy link

kenips commented Aug 20, 2018

@ljharb I know this isn't typical release style but considering the huge volume here could you please please please consider cutting a beta, next, or 1.3.0.beta-1 for ease of testing this? Thank you <3

ljharb added a commit that referenced this pull request Aug 25, 2018
 - Add `displayNameOfNode`, `isValidElementType` (#1701, @jquense)
 - pass the adapter into `createMountWrapper` (#1592, @jquense)
 - 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
 - Add pointer events support (#1753, @ljharb)
 - Add `displayNameOfNode`, `isValidElementType` (#1701, @jquense)
 - pass the adapter into `createMountWrapper` (#1592, @jquense)
 - 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 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] `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)
 - [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
@ljharb
Copy link
Member

ljharb commented Aug 25, 2018

enzyme v3.5 is released: https://twitter.com/ljharb/status/1033267583372292098

@kenips
Copy link

kenips commented Aug 25, 2018

Thank you @ljharb your great work is very much appreciated.

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.