childrenOf and childrenOfType fail with react-hot-loader

[liviudobrea]

Detailed description

react-hot-loader (v4.x.x) (will further refer to this as RHL) wraps every React component in a ProxyComponent, to internally handle the change / reload mechanism. That is all good, however, by using RHL with carbon-components-react, this statement https://github.com/carbon-design-system/carbon-components-react/blob/706584394a321c30c56367f1fee930a82fb26236/src/prop-types/childrenOf.js#L24-L28 and this statement https://github.com/carbon-design-system/carbon-components-react/blob/706584394a321c30c56367f1fee930a82fb26236/src/prop-types/childrenOfType.js#L20-L24 will always throw something similar to this:

warning.js:33 Warning: Failed prop type: Invalid prop `children` of type `ListBoxField` supplied to `ListBox`, expected each child to be one of: `[ListBoxField, ListBoxMenu]`.

Is this a feature request (new component, new icon), a bug, or a general issue?

This is not necessary an issue with the current code, but it becomes a bug when using this with the RHL 3rd party library (which I'm guessing many are).

This is an acknowledged issue with RHL. More details here gaearon/react-hot-loader#304.

Some Solutions

Depend on react-hot-loader

I'm not sure if this is a good enough reason to add RHL as a project dependency, but it provides the cleanest solution.

import { areComponentsEqual } from 'react-hot-loader';

...

if (
  !expectedChildTypes.includes(child.type) && 
  (
    typeof child.type === 'function' && 
    !areComponentsEqual(child.type, expectedChildTypes.find(c => c.name === childDisplayName)))
  )
)

Don't depend on react-hot-loader

This one's a tad uglier, but it also works. However, since RHL API could change at any given time, this is more likely to fail at some point.

(
  typeof child.type === 'function' &&
  Object.prototype.hasOwnProperty.call(
    child.type,
    '__reactstandin__getCurrent'
  ) && 
  child.type.__reactstandin__getCurrent() !== expectedChildType
)

More than happy to PR if you're ok with either of those.

[joshblack]

Hi @liviudobrea! 👋 I definitely wouldn't mind the first one given that it's the proper solution, in this case. My only concern would be if using that solution could potentially harm consumers who aren't using RHL?

If that's the case, wouldn't mind a general getProxyFor utility that encapsulates that behavior in the second example that you noted. It would also work as a place to hold logic for other libraries that might proxy React component, if that ever occurs.

[liviudobrea]

@joshblack Will run some intentional mis-usage test cases, with and without RHL, and report back.

[liviudobrea]

Implementation in #859

[liviudobrea]

Another take at it in #1253

[ollie-o]

@joshblack I noticed that both of the attempted solutions to this issue ended up not getting merged in.

Seems like #1253 in particular was super close to getting merged!

Are there plans for someone from the core carbon-react team to fix it? I get the impression that it could be solved pretty quickly if one of you picks up the issue and take over from where #1253 left things.

[joshblack]

@oliverodaa most likely we'll loosen our prop type definitions versus trying to fix it 😞 It's a very hard bug to fix which is why most PRs tend to die down.

Definitely feel free to contribute a fix based on 1253, was looking pretty promising 😄

[stale]

We've marked this issue as stale because there hasn't been any activity for a couple of weeks. If there's no further activity on this issue in the next three days then we'll close it. Thanks for your contributions.

[carbon-bot]

Hi there! 👋 If you're wondering why this issue was moved, we're currently updating our repo structure so that every package is found in the same project.

This should not have any impact for you, but we wanted to give you a heads up in case you were wondering what is going on. If you have any questions, feel free to reach out to us on Slack or contact us at: carbon@us.ibm.com. Thanks!