prioritize displayName over name

[toddw]

React provides the ability to override a component's display name by
using the displayName property. When displayName is provided it
should take precedence over name. Prior to this pull request,
displayName would only ever be used if name is not present but it
should have been the other way around.

[SimenB]

Sort of related, any chance of exposing the name on the (React/Shallow)Wrapper? Currently we do wrapper.type(), and optionally .name or .displayName after, but just doing .name() would be pretty sweet. Happy to provide a PR if you guys want

[ljharb]

I'm not sure if this PR constitutes a bugfix or a breaking change or both.

@SimenB as for .name(), that seems totally reasonable to me - it would also allow us to use that method internally instead of duplicating the fallback logic.

[JoeCortopassi]

The swap in ShallowTraversal.js#L168 seems less like a breaking change, and more like a reinstating of the more expected behavior in places like Debug.js#L24

[ljharb]

@JoeCortopassi my concern is that existing tests that use <Foo /> (to workaround the issue this is fixing) will start failing unless they change to use <CustomFoo />.

[JoeCortopassi]

@ljharb valid concern. Was there much pushback when the breaking change occurred in September? What broke then is what is likely to break now, and my guess is there wasn't a ton of outcry. I guess I'm just saying that fixing this brings consistency and seems better than purposefully supporting tests written with faulty/unintended behavior. Small release note would make patching this in existing code bases simple as well I imagine

[lelandrichardson]

I support this change... if there is a .displayName present, that is a string being set explicitly in the context of React and is likely the name that people want to use.

In the case of .name and .displayName being different, it would likely only be the case in minified code?

Am I thinking about this wrong?

[ljharb]

They'd be different in a React.createClass and a native arrow SFC, and they'd likely be different in a HOC.

[lelandrichardson]

@ljharb .displayName might be different here, but we are really talking about when the expression foo.name || foo.displayName is different from foo.displayName || foo.name though... An SFC wouldn't be different in this case unless the person explicitly used const Foo = () => {...}; Foo.displayName = 'Bar';, in which case since they are being explicit, it seems like 'Bar' is what they are really wanting in the first place.

In the case of React.createClass, can we confirm that this would produce a different result?

In the case where HOCs were different, it seems like this would also be "fixing a bug" in that it would go first for the more descriptive/explicit .displayName that the author of the HOC decided to go to the extra effort to produce (or otherwise it would fall back to .name and there would be no regression.)

Regardless of whether or not we consider this a patch or a breaking change, it seems clear to me that the desired behavior would be to use .displayName if it exists.

[ljharb]

I completely agree that displayName is the proper one to take precedence - I'm just not sure if we should treat it as major or patch, even tho it's conceptually a bug fix.

[toddw]

Is there a separate release schedule for breaking changes vs non breaking bug fixes? If this is considered to be a breaking change, does that mean a longer wait before it is merged?

In my opinion, this is clearly a bug. If people have written tests to rely on the results of this bug, then it will break their tests. Updating their tests should be really easy but if their build environment is set up to get the latest minor or patch version of the library automatically, some people might get upset that their tests are now broken without them manually changing anything. As much as I want to get this in so I can fix my broken tests, I am sympathetic to the concern. When this bug was added, was that a major version change? I assume that would have broken tests in the same way.

If a major version change doesn't take any longer, then I think it is reasonable to bump the version.

[toddw]

Since more and more people will be writing tests with the wrong assumptions for as long as this bug exists, I feel a sense of urgency to get the fix applied so that less people have to update their code once it is in. Also, less people will have issues with tests not doing what was expected once it is in.

[JoeCortopassi]

@ljharb I guess what I was trying to say earlier is this commit was the (unintentional) breaking change that caused a bug in existing systems, and @toddw has the bug fix in this PR.

Bug fixes by their vary nature break existing, but unintended, behavior (the bug) every time. I think the difference between that and what is more commonly referred to as a "Breaking Change" is the latter is identifying something that was purposefully done, and intentionally changing it to something new, that happens to not be backward compatible.

[lelandrichardson]

Looking at this PR more closely, I think I am leaning towards considering this a bug, and not a breaking change. This only affects the results of .text(), and every other usage of a component's name/displayName in the app does not follow this preference.

Initially, I thought that this would affect things like wrapper.find('Foo'), but I see now that it does not.

@ljharb thoughts?

[ljharb]

In that case I'm fine with it being a patch.

[ljharb]

@toddw would you mind rebasing on top of the latest master?

[toddw]

@ljharb no problem, rebase completed 👍