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

Add react-is package #12199

Merged
merged 19 commits into from
Feb 11, 2018
Merged

Add react-is package #12199

merged 19 commits into from
Feb 11, 2018

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Feb 9, 2018

Supersedes PR #11279 and #12092. Resolves issue #12038

Perhaps we could use this in React DevTools as well? (This would not be the case in its current form, since this library accepts React elements and DevTools receives fibers.)

Tests

In addition to the newly-added unit tests, I ran yarn build react-is locally to build this, then npm packed the result and installed it in a fresh project. I also tested the UMD build in CodeSandbox.

Example

import React from "react";
import {
  ContextConsumer,
  ContextProvider,
  isContextConsumer,
  isContextProvider,
  typeOf
} from "react-is";

const ThemeContext = React.createContext("blue");

isContextConsumer(<ThemeContext.Consumer />); // true
isContextProvider(<ThemeContext.Provider />); // true
typeOf(<ThemeContext.Provider />) === ContextProvider; // true
typeOf(<ThemeContext.Consumer />) === ContextConsumer; // true

Assumptions

  • It is okay that isElement is looser than typeOf (e.g. it returns true for host elements, fragments, strict-mode, context, etc.)
  • There are no additional component types (e.g. the experimental call/return types) that we want to expose for the initial release.
  • It's okay to support the (currently unstable) AsyncMode for the initial release.
  • This package should mirror the version of React rather than be versioned independently (since we'll be adding types over time).

@sebmarkbage
Copy link
Collaborator

I'm personally not a big fan of the typeOf mechanism, but not strongly enough to block it.

Another plausible approach is that we expose the symbols and typeOf returns a symbol. You can get the string name through String(typeOf(...)) and in the future, the symbol.description proposal.

Then you can switch on symbols instead of strings:

switch (typeOf(node)) {
  case ReactIs.Element:
    ...
  case ReactIs.Fragment:
    ...
}

@bvaughn
Copy link
Contributor Author

bvaughn commented Feb 10, 2018

Another plausible approach is that we expose the symbols and typeOf returns a symbol. You can get the string name through String(typeOf(...)) and in the future, the symbol.description proposal.

Wouldn't the issue still remain that we'd have to access the symbols from a different attribute/path depending on the type of element (e.g. object.type.$$typeof for context vs object.type for fragments vs object.$$typeof for portals).

Sorry if I'm missing or misinterpreting something.

@sebmarkbage
Copy link
Collaborator

The implementation of typeOf would work exactly the same. It would just return symbols as the return value instead of strings.

I don’t know if I can really motivate this very strongly though. Just feels more specific with symbols since they don’t take up string space but in practice strings are probably fine and more practical.

@bvaughn
Copy link
Contributor Author

bvaughn commented Feb 10, 2018

Oh, I see. I misunderstood your initial suggestion 😄

So you're saying rather than someone using e.g.

import React from 'react';
import ReactIs from 'react-is';

const ThemeContext = React.createContext('blue');

ReactIs.isContextConsumer(<ThemeContext.Consumer />);
ReactIs.isContextProvider(<ThemeContext.Provider />);

They would use:

import React from 'react';
import ReactIs from 'react-is';

const ThemeContext = React.createContext('blue');

ReactIs.typeOf(<ThemeContext.Provider />) === ReactIs.ContextProvider;
ReactIs.typeOf(<ThemeContext.Consumer />) === ReactIs.ContextConsumer;

I guess we'd still have to export the numeric fallback values for environments that don't support Symbol.for (like IE and RN Android)?

I'm not opposed to making this small change, if that's what's required for this lib to be shippable. 😄

@sebmarkbage
Copy link
Collaborator

I think using isContextConsumer is still best practice when you only test for a single value.

The typeOf is more about logging and switch statements where there are many possibly legit options. We stopped using isValidElement internally for the benefit of switch statements so seems like others might want to too. Although strings support that case too.

@bvaughn
Copy link
Contributor Author

bvaughn commented Feb 11, 2018

Cool. Seems like exporting constants for the string return-types would satisfy the switch/case use case as well.

I don't really have a strong preference. Gimme a few and I'll push an update. 😄

@bvaughn
Copy link
Contributor Author

bvaughn commented Feb 11, 2018

Okedoke. Rebased and added symbol exports.


// TODO: decide on the top-level export form.
// This is hacky but makes it work with both Rollup and Jest.
module.exports = ReactIs.default ? ReactIs.default : ReactIs;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should do this correctly right away since it's not a legacy package.

Let's remove the default export here and only provide named exports. That's how we make it work with tree shaking, too (when we provide an ES build output).

Copy link
Collaborator

Choose a reason for hiding this comment

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

To clarify, I'm saying this file shouldn't need to use CommonJS at all. Just re-export * from the source.

Copy link
Contributor Author

@bvaughn bvaughn Feb 11, 2018

Choose a reason for hiding this comment

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

Hm. Okay.

I'm curious: Why didn't we follow that pattern with other new packages (like react-reconciler)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there was some reason but I don't remember straight away.

It's not very important in case of the reconciler though because it provides just one function. react-is, on the other hand, provides a bunch, and is expected to grow.

Call/Return does avoid the .default hack:

module.exports = require('./src/ReactCallReturn');

If there's some issue with top-level ES import (maybe there was) then the above is sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. This makes sense. Thanks for the suggestion. 😄

It's been updated.

@bvaughn
Copy link
Contributor Author

bvaughn commented Feb 11, 2018

It looks like the Rollup build checks report an ESLint failure from the UMD builds:

/home/circleci/project/build/node_modules/react-is/umd/react-is.development.js
13:73 error 'exports' is not defined no-undef

The line in question is:

typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports) :

I believe we could fix the lint error (without breaking the UMD build) by changing the signature to:

typeof module === 'object' && typeof module.exports === 'object' ? factory(module.exports) :

But I'm not sure how to achieve this within the context of Rollup. 😄 I'm going to just remove the UMD build from the bundle. I added it because it seemed like a nice to have, but I don't think it's really very useful.

This was referenced Mar 30, 2018
rhagigi pushed a commit to rhagigi/react that referenced this pull request Apr 19, 2018
Authoritative brand checking library.

Can be used without any dependency on React. Plausible replacement for `React.isValidElement.`
@ljharb
Copy link
Contributor

ljharb commented Jun 26, 2018

@bvaughn isElement doesn't seem compatible with React 0.13 (enzymejs/enzyme#1538 (comment)) - any chance that's an easy fix?

@bvaughn
Copy link
Contributor Author

bvaughn commented Jun 26, 2018

Unknown. Have a minimal repro?

@gaearon
Copy link
Collaborator

gaearon commented Jun 26, 2018

Seems expected to me, #4832 didn't land until 0.14 so we didn't have $$typeof. Don't think we care about supporting 0.13 at this point.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jun 26, 2018

Ah, thanks Dan!

I'm not really familiar with React 13 or 14 😅

@gaearon
Copy link
Collaborator

gaearon commented Jun 26, 2018

Lucky you!

@ljharb
Copy link
Contributor

ljharb commented Jun 26, 2018

@gaearon in 0.13, isValidElement was https://github.com/facebook/react/blob/v0.13.3/src/classic/element/ReactElement.js#L290-L302 - if i made the tiny PR required to fall back to checking _isReactElement when $$typeof wasn't around, considering that fallback wouldn't require further maintenance, is there any chance about supporting it?

My hope is to migrate the final enzyme adapter using React.isValidElement to using react-is, if possible.

@gaearon
Copy link
Collaborator

gaearon commented Jun 26, 2018

It's an extra property lookup for a thing that isn't there (for cases when we normally return false), so I don't think we'll want to do this.

@ljharb
Copy link
Contributor

ljharb commented Jun 26, 2018

What if I came up with a way to determine if $$typeof was present or not at require time, so there'd be no runtime cost?

@sebmarkbage
Copy link
Collaborator

I don't know if we've articulated whether this package has any versioning guarantees. In particular I'd like to preserve the option to use an instanceof check or other brand check in the future without doubling the size of this package.

@ljharb
Copy link
Contributor

ljharb commented Jun 26, 2018

@sebmarkbage currently it works on 0.14, 15, and 16, fwiw. As far as extensibility - that's exactly why i want to use react-is instead of React.isValidElement everywhere - so that a package (like airbnb-prop-types) doesn't have to peerDepend on React in order to reliably detect elements. 0.13 support is the final piece (after some of the React.Children methods) I need to remove the React peerDep requirement from that package. Additionally, it'd be great to eliminate the one remaining enzyme adapter that calls React.isValidElement, just on a philosophical level.

I don't anticipate "0.13 support on isElement" to add more than a line or two, fwiw.

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.

5 participants