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.

Sign up for GitHub

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

Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
12d7a8c
Authoritative brand checking library
sebmarkbage Oct 19, 2017
f44c668
Patched up react-is and added some tests and docs
bvaughn Feb 9, 2018
0f92bd3
Updated react-is version number to mirror other packages
bvaughn Feb 9, 2018
c91ef08
Fix export for tests
bvaughn Feb 9, 2018
78c264d
Prettier
bvaughn Feb 9, 2018
cbffbd1
Add umd files to package
bvaughn Feb 9, 2018
88d1dd4
typeOf returns symbols; symbols exported for switch/case
bvaughn Feb 11, 2018
b8f67c2
README tweaks
bvaughn Feb 11, 2018
784fffc
Added broader isElement() tests
bvaughn Feb 11, 2018
7b060d3
Replaced default exports with named exports
bvaughn Feb 11, 2018
70bccd8
Added results.json
bvaughn Feb 11, 2018
b4ff653
Updated README
bvaughn Feb 11, 2018
54588e4
Removed UMD target from react-is bundle
bvaughn Feb 11, 2018
21a64bd
Documentation nits
bvaughn Feb 11, 2018
a06e6f7
Re-enabled UMD build for react-is
bvaughn Feb 11, 2018
b22a7b7
Made fragment/async/strict mode checks more strict
bvaughn Feb 11, 2018
ef0a900
Adjusted typeOf() to be more efficient
bvaughn Feb 11, 2018
f111111
Added a couple of additional tests
bvaughn Feb 11, 2018
9c8385c
Removed unnecessary, nested not-null check
bvaughn Feb 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Prettier
  • Loading branch information
@bvaughn
bvaughn committed Feb 11, 2018
commit 78c264d14acf8a273f1da51371a2e3c2766bdf08
4 changes: 1 addition & 3 deletions packages/react-is/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,4 @@ const ReactIs = require('./src/ReactIs');

// 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;
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 •
edited
Loading

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:

react/packages/react-call-return/index.js

Line 12 in 29e8924

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.