React: Do not resolve defaultProps in React.createElement

[mpodlasin]

Hi.

Do you want to request a feature or report a bug?
Feature request (I guess?).

What is the current behavior?

Currently defaultProps of component are resolved super early - when React.createElement is called. This makes shallow rendering for purposes of snapshot tests (with enzyme and enzyme-to-json - see https://github.com/adriantoine/enzyme-to-json/issues/55) problematic: If component A has some defaultProps, they will appear in snapshots of all components that use this component. This makes testing very fragile - every change of defaultProps should affect only tests of component A, but currently it will affect many other tests.

For example if some component uses A without any props, this is what I would expect in snapshot:

<div>
  <A />
</div>

but I am getting:

<div>
  <A someProps="some value from defaultProps" />
</div>

What is the expected behavior?

Resolve defaultProps at later stage, when they are actually needed, so that in snapshots appear only actually passed props.

Will appreciate feedback, thoughts, workarounds and possible alternative solutions.

Have a great day everyone! ;)

[bvaughn]

defaultProps directly affect a component instance's actual props and the props in turn affect the component's behavior/output. Given this, I think it makes sense that they influence snapshots in the way you described above.

I think it should be pretty easy to visually scan snapshots for diffs after changing defaultProps and then batch-update.

[mpodlasin]

See we disagree here. If I update Button component used in 1000+ components, I want to update 1 test, not go over 1000+ diffs to make sure everything is fine. ;)

Anyways - current implementation supports the way of testing you like, but not the way of testing I like. My question - is there a way to have both? If defaultProps were resolved later (perhaps after some API call is executed), we maybe could have both worlds - at the beginning I would have my defaults-free tree and after some API call defaults would be resolved and you would get your tree with defaults.

Perhaps we could have some flag that would turn resolution off, which could be set easily in the context of testing?

[bvaughn]

If you update defaultProps on a Button component that's used in 1,000 places- you potentially change that component's behavior in 1,000 places. Seems like snapshot tests should reflect this, no?

[mpodlasin]

But then one of this 1000 components is used in some other components. Button is used in Menu and Menu is used in Header. So I affected Headers behaviour as well - shouldn't snapshot reflect this as well? :P

The answer is no. This is why unit tests are called unit - I test only one element in separation. That's why mocks exists in tests. When I write test for some class, I mock it's dependencies because I want to test only this one class and how it behaves by itself.

[bvaughn]

Sorry but I don't understand. If you have a Header component that renders a Menu which renders a Button- why would a change to Button impact the _shallow_ snapshot output of Header? Menu shouldn't even be rendered, and so the Button element wouldn't even be created.

[mpodlasin]

Ok, maybe differently:

If change in Button should reflect in Menu, why do shallow render in the first place?

Why there is double standard between defaultProps and actual tree returned by Button? Both affect Menu after all.

[bvaughn]

Let's say you had a Button component that required a label prop:

Button.propTypes = {
  label: PropTypes.string.isRequired
};
Button.defaultProps = {
  label: "Click me"
};

And you had a Menu component that rendered a couple of Buttons. Your snapshot may look something like this:

<div>
  <Button label="Home" />
  <Button label="Contact Us" />
</div>

If the Button later changed its props so that it required a text property instead:

Button.propTypes = {
  text: PropTypes.string.isRequired
};
Button.defaultProps = {
  text: "Click me"
};

Then you'd want Menu tests to fail, right? Unless you updated Menu yourself- then it would no longer be configuring Button correctly. However if you stripped defaultProps from the snapshot output- you'd never know because your snapshot would remain:

<div>
  <Button label="Home" />
  <Button label="Contact Us" />
</div>

But in this case, the runtime behavior of your Menu component would be broken. (It would show a bunch of "Click me" buttons.)

[mpodlasin]

Hm... Ok. This actually sounds reasonable. For now consider me convinced. ;)
If you believe issue should be closed, I'll be ok with it. Thanks!

[bvaughn]

Thanks for the discussion @Podlas29!

I'll go ahead and close for now. To be honest, this is a decision you _could_ continue discussing with the Enzyme team though as (I'm pretty sure) they plan to implement their own, from-scratch shallow renderer for the next major Enzyme release. (This would give them the option of whitelisting defaultProps.)

[mxmul]

Hi all! Sorry to bump an old issue, but isn't the example above only unsafe if you don't have typechecking of your props?

We're using both snapshot tests and Flow types for testing our components, and it seems like changes to defaultProps in a shallow snapshot are purely noise. If the Button component in your example above was defined like so:

type Props = {|
    text: string,
|}

const Button = ({ text }: Props) =>
    <button>{text}</button>;

Button.defaultProps = {
  text: "Click me"
};

Then the Menu component would just fail to typecheck. I can't think of a good counter example where a change to a defaultProp is both a) something breaking, that should fail a shallow snapshot test and b) wouldn't be caught by Flow.

Curious what others think about this :)

[bvaughn]

I can't think of a good counter example where a change to a defaultProp is both a) something breaking, that should fail a shallow snapshot test and b) wouldn't be caught by Flow.

Many projects don't use Flow 😄

[mxmul]

Lol, fair enough. I just think there's value (for some projects at least, depending on their stack) in being able to exclude unspecified defaultProps from a shallow snapshot.

Maybe my use-case is uncommon enough to warrant a custom enzyme adapter though ¯\_(ツ)_/¯