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

[react-is] Add elementType API #12932

Closed
wants to merge 1 commit into from

Conversation

aweary
Copy link
Contributor

@aweary aweary commented May 29, 2018

Resolves #12882

Adds an API for checking what the element type would be if the value was passed to React.createElement. It's essentially isValidElementType except that it returns the element type.

@pull-bot
Copy link

Details of bundled changes.

Comparing: ff724d3...c40c389

react-is

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-is.development.js +12.8% +6.8% 4.67 KB 5.27 KB 1.29 KB 1.38 KB UMD_DEV
react-is.production.min.js 🔺+10.5% 🔺+4.6% 1.89 KB 2.08 KB 787 B 823 B UMD_PROD
react-is.development.js +13.4% +7.1% 4.49 KB 5.09 KB 1.23 KB 1.32 KB NODE_DEV
react-is.production.min.js 🔺+11.1% 🔺+5.8% 1.84 KB 2.04 KB 719 B 761 B NODE_PROD
ReactIs-dev.js +13.2% +7.0% 4.56 KB 5.16 KB 1.26 KB 1.35 KB FB_WWW_DEV
ReactIs-prod.js 🔺+13.8% 🔺+8.4% 3.67 KB 4.18 KB 999 B 1.06 KB FB_WWW_PROD

Generated by 🚫 dangerJS

@bvaughn
Copy link
Contributor

bvaughn commented May 30, 2018

cc @sebmarkbage

I don't think we (particularly Sebastian) want to expand the scope of this library.

Aside from the question of scope, I think the resulting API would be pretty confusing, e.g. elementType vs typeOf. If anything, I would expect to pass an element to the former and not the latter.

@aweary
Copy link
Contributor Author

aweary commented May 30, 2018

Yeah, Im not sure about the API name. I chose elementType because it accepts the same values as isValidElementType, so it felt consistent in that case.

@jquense
Copy link
Contributor

jquense commented Jul 2, 2018

This would be useful for enzyme as well: enzymejs/enzyme#1701

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

Introspection is bad. It breaks modularity since you can no longer safely wrap these. E.g. the compiler really wants this to be safe.

We are likely going to move more of these to actually return stateless functional component wrappers instead of symbols so that it is always safe to wrap them because the consumer can't introspect them.

case 'string':
case 'function':
return REACT_ELEMENT_TYPE;
case 'object':
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is very likely that we're going to do a breaking change and actually turn these into just functions.

@aweary aweary closed this Jul 2, 2018
@aweary aweary deleted the react-is-element-type branch July 2, 2018 23:53
@aweary
Copy link
Contributor Author

aweary commented Jul 3, 2018

There are existing use cases for this that are being implemented with even less safe introspection, so if those use cases are valid (enzyme, host-non-react-statics) than some form of introspection seems inevitable. Otherwise, alternative solutions that are safe and sanctioned need to be discussed.

@jquense
Copy link
Contributor

jquense commented Jul 3, 2018

Yeah I'm all about this being safer, but no introspection doesn't seem feasible for enzyme, dev-tools, etc. There is always s a tension between safety for "normal" users and hooks for those building tooling on top of and for React. Things like Enzyme not having to be baked into react is clearly a win for everyone, it'd be great if we could strike the right balance here.

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.

6 participants