update validation of component passed to the function returned by connect() #971
Conversation
src/components/connectAdvanced.js
Outdated
| @@ -2,6 +2,7 @@ import hoistStatics from 'hoist-non-react-statics' | |||
| import invariant from 'invariant' | |||
| import { Component, createElement } from 'react' | |||
| import { polyfill } from 'react-lifecycles-compat' | |||
| import * as ReactIs from 'react-is' | |||
There was a problem hiding this comment.
For the purposes of tree shaking, you should import only the isValidElementType function.
There was a problem hiding this comment.
@timdorr I did try, it fails when running npm run build
src/index.js → dist/react-redux.js...
[!] Error: 'isValidElementType' is not exported by node_modules/react-is/index.js
https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module
src/components/connectAdvanced.js (5:9)
3: import { Component, createElement } from 'react'
4: import { polyfill } from 'react-lifecycles-compat'
5: import { isValidElementType } from 'react-is'
^There was a problem hiding this comment.
any clues? here is the package: https://github.com/facebook/react/tree/master/packages/react-is
and the rollup wiki link: https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module
There was a problem hiding this comment.
For reference, here's the actual built file from the NPM package: https://unpkg.com/react-is@16.4.1/cjs/react-is.development.js .
The entire built react-is package is CJS, so I'm not sure there's any tree-shaking that's going to happen.
There was a problem hiding this comment.
AFAIK, Rollup and Webpack are working on tree-shaking/DCE for CJS bundles (well, all code, really).
But react-is is built exactly like react, so I'm not sure why that doesn't work. Maybe our build tools need an upgrade?
There was a problem hiding this comment.
@timdorr I don't want this PR to get stale, is there anything you recommend me doing? E.g. maybe creating a PR (or issue) to react-is so that it exports individual modules to enable tree-shaking
There was a problem hiding this comment.
as mentioned in #914 (comment) it's not yet possible to import specific portions of react-is, but if you fix the build of react-is, it would then become possible
There was a problem hiding this comment.
@timdorr @cellog ok, here is my attempt facebook/react#13250. "There goes nothing", I can't say I didn't try :)
There was a problem hiding this comment.
Cool. Let's see if they can get that out soon enough. I'm not particularly worried about this aging quickly, as this part of the code isn't often touched.
There was a problem hiding this comment.
I closed it because it was not the expected solution, also, it is over my head.
|
So do we still want to wait on It looks like We could also copy-pasta this function, I suppose, and just inline it. Here's the main source: https://github.com/facebook/react/blob/b565f495319750d98628425d120312997bee410b/packages/shared/isValidElementType.js . |
|
Note that per #914 and facebook/relay#2499 , |
|
Looks like there's some movement in facebook/react#13272 , so maybe we can wait for that to get worked out. |
that is a good question, but at this point, I feel like it hasn't moved forward because we should wait. Although no one has literally mentioned it. It looks like @TrySound is working on the ESM feature in facebook/react#13321. We are rooting for you TrySound! |
|
I do my best. For now you can use namedExports option of commonjs plugin. |
|
I'm waiting on facebook/react#13321 to update this PR. As soon as that get's merged I'll update the code. |
|
I'm waiting this one facebook/react#13356 |
|
Thanks, @TrySound! I didn't mean to make it sound like I was pressuring you, I just wanted to comment to let other developers know that this PR is not stalled. |
|
Rebased. |
|
Closed in favor of #1000 |
Fixes #914
This aims to remove the strict function check and use react-is to validate the component that is passed to the function returned by connect(). I first encountered this issue when I created an HOC that forwarded a reference to the composed component, as discussed in the issue referenced above.