Usage of .name|.displayName to identify stateless functional components

[jmm]

This is WIP. I want to see if this is something you're interested in and if so I can clean everything up.

1. Can support for displayName on stateless functional components be documented? (It would seem so from displayName is set to "o" when minifiying Stateless function components via webpack #4915 (comment).)
2. There seems to be some inconsistent application of .displayName / .name and I think there may be some holes (I'm only positive of one right now, the one in the failing test added here for the key warning on a stateless component with only .name.) Are you interested in patching up those holes?
3. If so, are you interested in consolidating that logic so it's applied consistently everywhere it's needed? That's the idea behind the getComponentName added here.

[jimfb]

To avoid lint error: var Something = function(){};

[jimfb]

This change seems good conceptually. I like the additional unit tests and docs change.

If so, are you interested in consolidating that logic so it's applied consistently everywhere it's needed? That's the idea behind the getComponentName added here.

We generally aren't afraid of a little copy-pasting. Premature consolidation can actually make readability worse (https://www.youtube.com/watch?v=4anAwXYqLG8). For instance, when reading the tests, I became a little frustrated when I hit testKeyAccess because I couldn't see what the test was doing just by reading the test, so I had to scroll up to the definition of testKeyAccess only to discover that it was doing something with opts.name but I couldn't remember what that was and so I had to scroll around two more times. Not a huge deal, but something to consider.

Anyway, I could go either way with getComponentName. Single line functions are a little questionable, IMO. I'd just as soon fix it inline within ReactElement.js, especially since we've already pulled out many of the accessors in places like ReactCompositeComponent.getName, it means there would only be a few callsites of getComponentName.

[jimfb]

Also, you have a few lint errors, try npm run lint

[facebook-github-bot]

@jmm updated the pull request.

[jmm]

Thanks for the review and feedback @jimfb. I'm sorry the lint errors were a distraction. My goal was to provide a good enough sketch of what I had in mind to see if it's something you're interested in before doing all of the work. In the future I'll try to eliminate lint errors first so they're not distracting. (To that end, it'd be helpful if master linted.)

I've pushed new commits here that resolve the lint errors and perhaps address some of your other concerns (discussed below).

Docs

How about:

.displayName or .name may be used as the name of the component within debug messages.

Removes " (in that order)" and replaces "will" with "may". The reason being that we are also going to start pulling name info from the jsx source transform, so we don't want to be making guarantees about how component names are derived.

I ended up seeing some discussion of that after opening this PR, so I know what you mean and I understand not wanting to over commit, but I would think that at least specifying that displayName would take precedence over name would make sense. Worded something like:

.displayName or .name may be used as the name of the component within debug messages if it's a non-empty string. If so, .displayName would take precedence over .name.

What do you think?

Test structure

We generally aren't afraid of a little copy-pasting. Premature consolidation can actually make readability worse (https://www.youtube.com/watch?v=4anAwXYqLG8).

I guess I'm not going to get away with not watching this video....

For instance, when reading the tests, I became a little frustrated when I hit testKeyAccess because I couldn't see what the test was doing just by reading the test, so I had to scroll up to the definition of testKeyAccess only to discover that it was doing something with opts.name but I couldn't remember what that was and so I had to scroll around two more times. Not a huge deal, but something to consider.

I'm sorry that was frustrating. In my defense:

• I would've just made the warning message part dynamic (instead of duplicating it n times) and had a single test with a few assertions, except that for the assertions to work the module registry needs to be cleared in between, so I split it into a few tests, with most of the logic consolidated in testKeyAccess() as you saw, to let the beforeEach take care of that.
• I deleted a decent chunk of code (Parent) that doesn't appear to be necessary for the test, simplifying the actual code that sets up state for the assertions.

Personally I'm not a big fan of duplication. If something has to be duplicated I'd prefer it to be some comments than all of the code, which can be the same for each instance except for a couple of parameters. I've added some comments documenting the functions like testKeyAccess(), documenting the call sites, and improved the descriptions of the tests. (If I don't have exactly the right format for the API doc comments I can fix that.) Perhaps that'll make you feel better about how it's structured, but if not, it's up to you.

Component name helper function

Anyway, I could go either way with getComponentName. Single line functions are a little questionable, IMO. I'd just as soon fix it inline within ReactElement.js, especially since we've already pulled out many of the accessors in places like ReactCompositeComponent.getName, it means there would only be a few callsites of getComponentName.

Ultimately it's up to you, and of course you're far more familiar with the code base than I am. I haven't gone through all of the potential call sites (so I'm not even sure how many there are), but there are 2 inconsistent versions I know of: use .displayName if it's in type, or else fall back on a static default, or use .displayName if it's truthy or else fall back on .name if it's truthy then a static default. The most important benefits of extracting it to a function would be to apply consistent logic everywhere it's needed with a single point of maintenance for future changes. Considering ReactCompositeComponent as an example, there are 4 cases of Component.displayName || Component.name || 'Component' inline just in that file, in addition to the logic in getName().

I have to take a closer look at what getName() does, but seeing that and hearing about the plans for embedding name data via the JSX transform makes me wonder if a function more generalized than what I have here for getComponentName() would make sense. Maybe something that can take an element, class, or component, and maybe a hint about the context, and return the correct name(s).

[jimfb]

The "if so" is a little confusing. How about:

.displayName or .name may be used as the name of the component within debug messages if it's a non-empty string. .displayName would take precedence over .name when calculating the component name.

[jmm]

@jimfb

The "if so" is a little confusing. How about:

👍 Edited that in here.

[facebook-github-bot]

@jmm updated the pull request.

[jimfb]

This is WIP. I want to see if this is something you're interested in and if so I can clean everything up.

Is this now ready, or are there more changes that you wanted to make?

Also, please squash your commits together into a single commit (git rebase -i, git push -f).

[jmm]

@jimfb So you're good with the structure of the tests and with adding the getComponentName() helper?

There are probably some more changes I'd make, though it should work to merge this and PR those separately later, or add them to this PR depending on your preference. Some of the things I have in mind:

• Adding the getComponentName() helper would make the most sense if inline instances of stuff like Component.displayName || Component.name || 'Component' (like the 4 I mentioned are in are ReactCompositeComponent) are updated to use the helper instead, though it's not strictly necessary that they all be updated to begin using it. Is that something you'd want done? I know that sweeping changes deserve scrutiny, so I don't know if you'd rather give other maintainers more opportunity to review or anything.

• The adjacent props.ref warning should probably get the same treatment (tests and use of the helper or whatever).

• Originally I was going to PR the docs update and add a test to renderers/shared/reconciler/__tests__/ReactStatelessComponent-test.js for the unique "key" prop warning on a stateless component with displayName to prove that it worked (which it did in that case), then I discovered the other cases where it was inconsistent. It'd probably still be a good idea to add that test.

• If the helper is going in I should fix this description:

  @param {ReactComponent} component

  Should be ReactClass and say something about accepting a stateless component function too.

I was hoping you could help me understand something about the tests. In getComponentName-test I thought I'd have to tell it not to mock the module, but that didn't appear to be necessary. I started looking into it but didn't track down why.

Also, please squash your commits together into a single commit (git rebase -i, git push -f).

Sure, I'll do that when this settles. (Aside: when squashing a whole branch I usually find it easier to do something like git checkout -b whatever-squashed master; git checkout whatever -- .)

[gaearon]

Having many tests like this seems unnecessary to me. If we extract getComponentName into a function, we can get away with one test here and a bunch of tests for specific cases in getComponentName-test. The indirection with testKeyAccess seems over-abstracted and unnecessary to me. Can we make this file simple again?

[aweary]

This PR has heavy conflicts with changes made in #7488 and #8149. getComponentName now already exists in the codebase and supports Fiber as well. Since much of this work is out of date and there's been no response to feedback I'm going to close this out.

@jmm feel free to open a new PR if you want to work on this, it looks like all it would take is using the existing getComponentName in ReactElement.