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

ReactIs.typeOf for non-elements #12882

Closed
mridgway opened this issue May 22, 2018 · 13 comments
Closed

ReactIs.typeOf for non-elements #12882

mridgway opened this issue May 22, 2018 · 13 comments
Labels

Comments

@mridgway
Copy link
Contributor

mridgway commented May 22, 2018

What is the current behavior?

ReactIs.typeOf currently only works for element types, it does not allow you to pass a raw Component or ForwardRef to know the type.

The use case for this is in hoist-non-react-statics I now need a special cases for ForwardRefs. To do this, I will need to know the type of the source and target components, but currently I would need to turn them into elements first.

All of the ReactIs.is* functions also have this issue since they use the typeOf function internally.

const ForwardComponent = React.forwardRef(() => {});

ReactIs.typeOf(ForwardComponent); // undefined
ReactIs.typeOf(React.createElement(ForwardComponent)); // Symbol(react.forward_ref)

What is the expected behavior?

Ideally I could pass in just the Component and get the type of it:

ReactIs.typeOf(ForwardComponent) // Symbol(react.forward_ref)

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

[email protected]

@aweary
Copy link
Contributor

aweary commented May 26, 2018

Hey @mridgway, thanks for bringing this up. As I understand it, the intention with ReactIs.typeOf is that it reads the types of different elements; it's meant to type check values that are returned from React.createElement. There's also ReactIs.isValidElementType which tells you whether a value can be passed to React.createElement. So there's a certain symmetry there.

Changing typeOf to return the type for the ForwardRef element type and ForwardRef elements could create problems if anyone relies on typeOf to only return values for things that can be rendered (things returned from React.createElement). Maybe that's OK in this case, I'm not sure.

It sounds like what you want is a hybrid of the two that tells you the type for element types.

React.elementType('div')
// Symbol(react.element)
React.elementType(React.StrictMode)
// Symbol(react.strict_mode)
React.elementType(React.forwardRef(Component))
// Symbol(react.forward_ref)

Do you think an API like that would suffice for your use case(s)? @bvaughn what do you think?

@bvaughn
Copy link
Contributor

bvaughn commented May 26, 2018

As I understand it, the intention with ReactIs.typeOf is that it reads the types of different elements; it's meant to type check values that are returned from React.createElement.

That's correct.

I can see the usefulness of a method like the one @aweary describes. I don't really have strong thoughts about it other than sharing Brandon's concern about it being a backwards breaking change to make to the existing react-is methods.

@mridgway
Copy link
Contributor Author

mridgway commented May 29, 2018

@aweary Yes, I think that API would be sufficient for my use case. Specifically I would be doing something like if (React.elementType(React.forwardRef(Component)) === ReactIs.ForwardRef). Any way that I can do this without having to reach in to Component['$$typeof'] would be fine with me.

@bvaughn
Copy link
Contributor

bvaughn commented May 29, 2018

Any way that I can do this without having to reach in to Component['$$typeof'] would be fine with me.

ReactIs.isForwardRef(
  React.createElement(
    YourComponent
  )
)

🤡

@mridgway
Copy link
Contributor Author

mridgway commented May 29, 2018

Yeah, that's what I will do for now. It seems a bit roundabout though and also introduces a dependency on React that I wouldn't need if ReactIs contained all of the logic.

Edit: Just realized that was a clown icon. It did not show up on my Mac 😞

@aweary
Copy link
Contributor

aweary commented May 29, 2018

I took a stab at implementing elementType #12932

@mridgway
Copy link
Contributor Author

mridgway commented Jul 2, 2018

Checking in on this again. Since it does not seem like there is movement on adding an API to be able to check types without creating elements, I will be implementing my changes using React.createElement.

This brings up the question of what the compatibility for react-is is currently and what its plans are in the future. If I need to add react-is and react as dependencies, can I loosely version react but tightly version react-is or will react-is continue to version in lock-step with react? Which previous versions of react does react-is support?

@bvaughn
Copy link
Contributor

bvaughn commented Jul 2, 2018

Which previous versions of react does react-is support?

Should support versions 0.14+

It relies on $$typeof (added in 0.14), so it will not support <= 0.13

@mridgway
Copy link
Contributor Author

Looks like using React.createElement is also problematic since it creates warnings if you don't have the correct props added.

@mweststrate
Copy link

mweststrate commented Nov 20, 2018

For future inspiration / real-life use case / other running into this as well:

In the mobx-react bindings some special forward ref handling was required as well, but react-is turned out to be not really valuable as it works on elements, not on types. Since the ForwardRef symbol isn't officially exposed from the package (or at least it become undefined after using rollup, didn't investigate further), in the end it was solved like:

https://github.com/mobxjs/mobx-react/blob/a3e9d3928015511bcb14846aad4de21a219ed39a/src/observer.js#L30-L31

const ReactForwardRefSymbol =
    typeof forwardRef === "function" && forwardRef((_props, _ref) => {})["$$typeof"]

and:

https://github.com/mobxjs/mobx-react/blob/a3e9d3928015511bcb14846aad4de21a219ed39a/src/observer.js#L329

if (ReactForwardRefSymbol && componentClass["$$typeof"] === ReactForwardRefSymbol) {
  // something specific
}

Doesn't feel entirely kosher, especially the dependency on $$typeof, so a suggestion for better alternatives would be appreciated but this does seem to do the trick for now.

(N.B.: there is an interesting underlying reason for the special case handling of refs, the bindings are "unpacking" the forward ref to make sure observer is applied as first HoC, normally that responsibility is moved to user land; but with forward refs that can't be really done (ForwardRefs are often produced by other libs and the user is unaware of them). If this problem is worth discussing I could open another issue)

@eps1lon
Copy link
Collaborator

eps1lon commented Nov 27, 2018

A solution to the specific use case of @mridgway would be to make those properties non-configurable and non-writeable. It would help a lot if this would also be considered for future elements (React.memo has already some issues with hoisting until #14313 gets merged). However this should be done in prod mode too to work with hoist-non-react-statics.

@stale
Copy link

stale bot commented Jan 10, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 10, 2020
@stale
Copy link

stale bot commented Jan 17, 2020

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants