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

[RFC] Add react-is package #11279

Closed
wants to merge 1 commit into from
Closed

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Oct 19, 2017

Authoritative cross-realm brand checking library. Can be used without any dependency on react. Plausible replacement for React.isValidElement.

Builds on the idea of https://github.com/jasnell/proposal-istypes

"main": "index.js",
"repository": "facebook/react",
"engines": {
"node": ">=0.10.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

We support Node 0.10?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copypasta from https://github.com/facebook/react/blob/master/packages/react/package.json#L20-L22

I guess we don't include that in other packages..?

@trueadm
Copy link
Contributor

trueadm commented Oct 19, 2017

Are we going to support a isReactClassComponent?

```js
import React from 'react';
import {createPortal} from 'react-dom';
import {isFragment} from 'react-is';
Copy link
Contributor

Choose a reason for hiding this comment

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

s/isFragment/isPortal

Can be used without any dependency on React. Plausible replacement for
React.isValidElement.
@sebmarkbage
Copy link
Collaborator Author

isReactClassComponent is a little bit different from the others.

It's not part of the "Node" types. Although that begs the question if we should have isNode too.

I also think that in general it is bad practice to treat component types as something special because if you do, it means that you can't accept other types like functional components or possible future module components. That said, we do treat them as something special inside React itself anyway.

@sebmarkbage
Copy link
Collaborator Author

Btw, I think that ideally the isBuiltIn proposal would be combined with the Symbol.matches mechanism in the Pattern Matching Proposal.

We could expose these matchers on placeholder constructors so that you could do something like this to match:

match (x) {
  React.Element: ...,
  React.Fragment: ...,
  React.Portal: ...,
  React.Component: ...,
}

@MatteoVH
Copy link

This is a good idea! My two suggestions would be:

  1. Add tests. Ensure that a <div /> is actually recognized as an Element, a ReactDOM.createPortal(...) as a Portal, etc.

  2. Doesn't have to be done in this PR, but I think it would be a good time to tackle this: clean-up the references to all the different types of React elements in the React package and import those from there so that there's a single source of truth for symbols/magic numbers and so that changes in the React codebase to the symbols or the numbers don't break this package.

@gaearon
Copy link
Collaborator

gaearon commented Oct 26, 2017

We should probably get back to this after ES modules. Since then it's less annoying to import this package from internals. Tree shaking ftw.

@aweary
Copy link
Contributor

aweary commented Nov 28, 2017

ES modules are done, is this still something we want to do?

@sebmarkbage
Copy link
Collaborator Author

@gaearon I didn't expect that we would import this from internals. This would just be stand alone package as a convenience that we don't depend on.

@ljharb
Copy link
Contributor

ljharb commented Dec 5, 2017

It would be nicer if it was imported internally, but as long as it wa guaranteed to keep apace with React internals that’s the same imo.

@aweary
Copy link
Contributor

aweary commented Dec 5, 2017

Talking with @ljharb, there are some internal methods like getComponentName that would be useful for third-party libraries. Could we consider a broader approach for general introspection utilities, like react-introspection?

We expose ReactFiberTreeReflection via react-reconciler/reflect, so maybe it would be possible to unify reflective and introspective utilities too?

@gaearon
Copy link
Collaborator

gaearon commented Dec 6, 2017

I like this idea.

@sebmarkbage
Copy link
Collaborator Author

Reflection is always where I get nervous. It has so much benefit and zero downside if everything stays the same forever... and that's where it is dangerous.

@ljharb
Copy link
Contributor

ljharb commented Dec 6, 2017

For “get component name” and “is valid element”, having a separate package that doesn’t depend explicitly on React creates conveniently used abstraction points so that if React ever does change the meaning, people can transparently get the updates. I don’t see either of those as reflection, tbh.

@aweary
Copy link
Contributor

aweary commented Dec 6, 2017

I believe we already expose the only reflective utilities that would be useful to expose, so react-introspection would just include some new introspective APIs and offer a chance to centralize introspective and reflective utilities, if we wanted to.

For context, Enzyme uses react-reconciler/reflect but it doesn't use react-reconciler at all, so that might mean that isn't the ideal place for it.

@gaearon
Copy link
Collaborator

gaearon commented Dec 6, 2017

To be clear I’d only include the truly safe ones. For example getComponentName is trivial but annoying to copy paste everywhere.

@ljharb
Copy link
Contributor

ljharb commented Dec 6, 2017

isValidElement would allow airbnb-prop-types to not have to depend explicitly on React, and it could remove the peer dep. getComponentName would allow me not to have to re-invent this utility in about 12 different packages (I've resisted publishing it separately myself, because the value of that primarily exists when React commits to cleave to it)

@sebmarkbage
Copy link
Collaborator Author

@gaearon getComponentName isn't safe. When we started minimize class names at FB it broke a bunch of components in production that used the name for things like keys in maps. It'll also be confusing with the component folding which I guess should intentionally strip names to avoid further confusion.

How do we avoid such uses making their way into popular libraries? Not saying people won't do it anyway using .name or .displayName but there is some risk involved with making it an official API. More so with other things.

@aweary The things we do today are already problematic and not an argument that we should cement them further. E.g. several things on ReactFiberTreeReflection we know that we'll need to deprecate and Enzyme's usage of them is a prime example of how popular libraries makes it harder for the ecosystem to do so. I don't know if we can justify the amount of time we spent on that for 16 again. Sometimes there are legit problems that can't be solved any other way and sometimes it just requires a few more back and forths to create a design that doesn't need that introspection.

@sebmarkbage
Copy link
Collaborator Author

isValidElement is an example that is legit since it's something that you're already expected to use in production behavior and the internal semantics require. Even that has some problems because there are some compiler optimizations that are impossible because we make the internals of "children" observable, but current plans don't include an approach to combat that problem so it's probably fine.

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

If this is written with our regular package structure I'd be cool with releasing it.

@bvaughn
Copy link
Contributor

bvaughn commented Feb 9, 2018

Superceded by PR #12199

@bvaughn bvaughn closed this Feb 9, 2018
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.

9 participants