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

Fix memo and lazy element types being considered elements #14546

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@eps1lon
Copy link

eps1lon commented Jan 8, 2019

ReactIs.typeOf is currently returning an element type for lazy and memo even if the given value is not an element (i.e. return value of React.createElement) but and elementType. This is confusing since @sebmarkbage spoke against an elementType API in #12932 (review).

This means that it is currently possible to determine if a given elementType is a lazy or memo component.

This PR changes the following behavior so that lazy and memo behave similar to e.g. forwardRef when passed to is* or typeOf.

// current
const Forward = React.forwardRef(() => null);
const Memo = React.memo(() => null);

ReactIs.isValidElementType(Forward) === true;
ReactIs.isForward(Forward) === false;
ReactIs.isForward(<Forward />) === true;

ReactIs.isValidElementType(Memo) === true;
ReactIs.isMemo(Memo) === true;

// after
ReactIs.isMemo(Memo) === false;
ReactIs.isMemo(<Memo />) === true;

I just want to add that I would prefer if ReactIs.typeOf(Memo) and ReactIs.typeOf(FowardRef) would return the element type but as far as I understood @sebmarkbage both should return undefined.

Pinging @ljharb since it sounded like (#14313 (review)) that would be a breaking change for enzyme.

Related:
#12882 (comment)

typeOf checks the return value of createElement

or ReactDOM.createPortal.

@ljharb
Copy link
Contributor

ljharb left a comment

It would indeed be a breaking change for enzyme; it’s critical that enzyme (and presumably react dev tools) be able to distinguish lazy and memo, both from each other and from unwrapped elements.

Neither memo nor lazy actually tries to be an invisible wrapper - own properties aren’t copied over, including defaultProps, and including the displayName. In other words, imo if it’s possible to differentiate at runtime, then it must be ergonomic to differentiate using react-is.

@eps1lon

This comment has been minimized.

Copy link

eps1lon commented Jan 8, 2019

So from my understanding there is is currently an asymmetry between e.g. React.forwardRef and React.memo with regards to React.is*. Before 16.7 we had the following signature: React.isType(value: any): value is React.ReactElement which basically meant that we would get something like React.isForwardRef(React.createElement(Component)) === true.

ReactIs.isMemo now accepts element types (or React.ReactComponentType if you're familiar with the typescript declarations for react) instead of elements which was rejected for React.forwardRef because "introspection is bad". So I'm curious why this introspection is ok for lazy and memo element types but bad for forwardRef types.

@ljharb react-devtools has no dependency on react-is. Looks like they're directly using the symbols: https://github.com/facebook/react-devtools/blob/4d30604d971b6969982c1346748c6721def0e618/backend/attachRendererFiber.js

Neither memo nor lazy actually tries to be an invisible wrapper - own properties aren’t copied over, including defaultProps, and including the displayName.

Which is also true for forwardRef so I don't think that argument applies here.

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Jan 8, 2019

I think the introspection should also be possible for forwardRef types.

@Jessidhia

This comment has been minimized.

Copy link

Jessidhia commented Jan 9, 2019

forwardRef is typically given a trampoline wrapper function (which is just a function component with a 2nd argument), while memo gets a component directly; that makes memo much easier to introspect.

As for lazy, it probably can't be introspected other than by attaching to its Promise.

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Jan 10, 2019

Simply knowing that it's "lazy" - and not just a regular old Promise - would be sufficient introspection for enzyme (although providing any sort of info, like a name, would be super helpful too).

@eps1lon

This comment has been minimized.

Copy link

eps1lon commented Jan 10, 2019

I'm not really following that hole introspection is bad/ok argument. Can you link me some resources that explain that concept and why it might be bad for some compilers?

I just can't see the pattern that disallows ReactIs.isforwardRef(Component) but allows ReactIs.isMemo(Component).

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Jan 10, 2019

That there's a current lack of consistency doesn't mean the right direction is removing functionality that enzyme depends on.

@eps1lon

This comment has been minimized.

Copy link

eps1lon commented Jan 10, 2019

That there's a current lack of consistency doesn't mean the right direction is removing functionality that enzyme depends on.

I agree with that but I all did was follow the documented path that is currently ignored in the implementation. I'm all for a is*Type or elementType API similar to #12932.

Edit:
just to further clarify if I'm opening a PR it's not that I want that particular change. I just think it makes discussion easier on github.

@eps1lon eps1lon referenced this pull request Jan 10, 2019

Merged

react-is: Add 16.7 features #31968

9 of 9 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment