babel-plugin-flow-react-proptypes icon indicating copy to clipboard operation
babel-plugin-flow-react-proptypes copied to clipboard

Void Union Prop Types marked as Required

Open ghickman opened this issue 6 years ago • 4 comments

When setting up a prop type as:

foo: Foo | void

I get the following warning in my console:

Warning: Failed prop type: The prop foo is marked as required in MyComponent, but it's value is undefined.

I can make the warning go away by changing the prop definition to:

foo? Foo

but of course this changes the meaning of my type def.

ghickman avatar Sep 12 '17 15:09 ghickman

To generalize the example a bit:

{
  foo: x | y | null,  // will correctly be marked optional
  bar: x | y | void,  // won't be marked optional, but it should
}

nvie avatar Sep 13 '17 06:09 nvie

Thanks for reporting this. Sorry for the delay, fixed in 5.1.3.

New behavior:

Input:

var React = require('react');

type Props = {
  foo: string | void,
};

function C(props: Props) {
  return <div>{props.foo}</div>;
}

Output

"use strict";

var React = require("react");

function C(props) {
  return React.createElement("div", null, props.foo);
}
C.propTypes = {
  foo: require("prop-types").string
};

Please give it a try and let me know if there are any issues!

brigand avatar Sep 14 '17 18:09 brigand

Thanks for the quick fix, @brigand – really appreciated! After reading the patch I was convinced it would work, and it does… as long as we use the union syntax with the void literal in it. However, in our code base, we're using a custom type alias for this, defined as follows:

type Maybe<T> = T | void;

Which still makes this a problem for us, as the AST traversal will not take the UnionTypeAnnotation branch directly. I assume it's travelling via the GenericTypeAnnotation instead, and not able to "transfer" the now-correctly found optional propType from within the nested AST node to the outside of it.

Looking at your test case and adjusting it slightly, I believe this test would demonstrate the bug:

const babel = require('babel-core');
const content = `
var React = require('react');

type Maybe<T> = T | void;

type Props = {
  foo: Maybe<string>,
};

function C(props: Props) {
  return <div>{props.foo}</div>;
}
`;

it('union-void-through-generic', () => {
  const res = babel.transform(content, {
    babelrc: false,
    presets: ['es2015', 'stage-1', 'react'],
    plugins: ['syntax-flow', require('../')],
  }).code;
  expect(res).toMatchSnapshot();
});

nvie avatar Sep 15 '17 06:09 nvie

@nvie it seems we don't support type X<T> at all. I don't have time to implement it, but I'd love a PR.

brigand avatar Sep 15 '17 20:09 brigand