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

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

Closed
majapw opened this issue Mar 20, 2017 · 10 comments
Closed

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

majapw opened this issue Mar 20, 2017 · 10 comments
Assignees
Labels

Comments

@majapw
Copy link
Collaborator

majapw commented Mar 20, 2017

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 ljharb added the bug label Mar 20, 2017
@ljharb
Copy link
Owner

ljharb commented Mar 22, 2017

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
Copy link
Collaborator Author

majapw commented Mar 22, 2017

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

@majapw
Copy link
Collaborator Author

majapw commented Mar 22, 2017

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

@ljharb
Copy link
Owner

ljharb commented Mar 22, 2017

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

@ljharb ljharb self-assigned this Mar 22, 2017
ljharb added a commit that referenced this issue Mar 24, 2017
ljharb added a commit that referenced this issue Mar 24, 2017
ljharb added a commit that referenced this issue Mar 24, 2017
@Nantris
Copy link

Nantris commented Sep 15, 2021

@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
Copy link
Owner

ljharb commented Sep 15, 2021

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

@Nantris
Copy link

Nantris commented Sep 15, 2021

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
Copy link
Owner

ljharb commented Sep 15, 2021

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
Copy link

Nantris commented Sep 16, 2021

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
Copy link
Owner

ljharb commented Sep 16, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants