ljharb / prop-types-tools

Custom React PropType validators
MIT License
671 stars 50 forks source link

`explicitNull` does not work when part of `PropTypes.oneOfType` #12

Closed majapw closed 7 years ago

majapw commented 7 years ago

Things that work as expected: foo: explicitNull().isRequired breaks for anything that is not foo={null}, including foo={undefined}. foo: PropTypes.oneOfType([explicitNull()]).isRequired breaks on foo="foo" foo: PropTypes.oneOfType([explicitNull().isRequired]) breaks on foo="foo"

Things that do not work as expected: foo: PropTypes.oneOfType([explicitNull()]).isRequired breaks on foo={null} foo: PropTypes.oneOfType([explicitNull().isRequired]) passes on foo={undefined}

ljharb commented 7 years ago

I would expect foo: PropTypes.oneOfType([explicitNull().isRequired]) to allow undefined, because an optional oneOfType would always allow both null or undefined.

Similarly, I think foo: PropTypes.oneOfType([explicitNull()]).isRequired is failing on null because the outer isRequired is failing on null.

I'm not sure this can be fixed; what happens if instead of oneOfType, you use or?

majapw commented 7 years ago

or doesn't work either, but also it seems to rely on oneOfType under the hood so I am unsurprised. :/

majapw commented 7 years ago

I think we would have to explicitly iterate through each of the validators, make sure one passes and go from there.

ljharb commented 7 years ago

Interesting, OK - seems like this is a quirk of explicitNull that's exposing an actual bug in or.

Nantris commented 3 years ago

@ljharb any idea why a PropType like this wouldn't work?

  const attemptOne = or([node.isRequired, explicitNull().isRequired]).isRequired;
  const attemptTwo = or([node, explicitNull().isRequired]).isRequired;

The tests you included cover those cases, and I assume they pass since no one has mentioned this in 4 years, but still we see:

Failed prop type: InnerPopper: invalid value Error: Invalid prop referenceElement of type object supplied to InnerPopper, expected an array.,Error: Invalid prop referenceElement supplied to InnerPopper, expected a ReactNode.,TypeError: InnerPopper: prop “referenceElement” must be null; received object supplied to required referenceElement.

referenceElement is equal to null at the time this error appears

Any idea what we might be doing wrong here? I know typeof null === 'object', but this still seems unexpected.

ljharb commented 3 years ago

@Slapbox what version of react are you using? make sure npm ls exits zero. Can you share the full code of InnerPopper?

Nantris commented 3 years ago

Wow what a response time! Thank you for your reply @ljharb!

npm ls lists dozens of lines of npm ERR! .... but I think this is expected since we're using yarn with workspaces in a monorepo.

yarn list exits simply with Done in 3.59s and everything listed seems to be in order.

Our React version is 17.0.2 and airbnb-prop-types is on the 2.16.0

Unfortunately I can't share the code of InnerPopper in full but I'll share anything I can if any possibilities come to mind.

The parent of InnerPopper (massively simplified) looks like this:

  const [referenceElement, setPopperReferenceElement] = React.useState(null); 
  <InnerPopper
     referenceElement={referenceElement}
  />

I'm not sure what I can share that might be useful, but I never see the value set to anything other than null or a valid DOM node at any point in execution ¯\_(ツ)_/¯

ljharb commented 3 years ago

Note that airbnb-prop-types doesn't support React 17 yet (see #73) but i doubt that's the issue here.

Given that our tests pass, I'm pretty confused what the issue would be, and I'm not sure how to figure it out.

The "expected an array" is strange - what's InnerPopper.propTypes look like?

Nantris commented 3 years ago

Ah I didn't realize that React 17 wasn't officially supported, thanks for the tip!

Really hate to waste your time, because of course the issue was on our end! Sorry @ljharb, but a big thanks for all you do!


TLDR: Not a problem with explicitNull(). We export a custom propType for popperReferenceElement but it wasn't working as intended. I thought we'd only used it in one place, and so, to reduce the number of variables in my debugging I replaced the importable PropTypes.popperReferenceElement with the propTypes mentioned above. Unfortunately we did use it in another place, so no amount of changes I made were affecting the components throwing the error. I could list some excuses, but at the end of the day, should have read the call stack more carefully.

Additionally, PropTypes.node didn't work as expected for this case. Switching to PropTypes.instanceOf(Element) and making sure all of the relevant places used that code worked as intended.


In case it helps someone in the future, we are now using:

  or([instanceOf(Element), explicitNull().isRequired])
ljharb commented 3 years ago

PropTypes.node looks for React elements, instanceOf(Element) looks for DOM elements - the two are quite different. Glad you figured it out!