pass through the displayName from the mocked component #21429

quantizor · 2018-09-30T20:46:00Z

Without this, most tooling will just say "Component" instead of "Text", "View" etc.

Test Plan:

Copied the change over and verified that it worked locally in the styled-components tests.

Release Notes:

[INTERNAL] [BUGFIX] [jest/mockComponent.js] - resume passing the mocked component's displayName through so it will appear in downstream tooling

without this, most tooling will just say "Component" instead of "Text", "View" etc.

elicwhite · 2018-10-01T00:00:28Z

Thanks for contributing! This looks reasonable to me at first glance, but it is hard to tell from your test plan how I can reproduce the issue you are fixing. Can you provide a minimal jest test that reproduces this error if your fix wasn't included? Better yet, can you add a jest test for mockComponent to the codebase to ensure this keeps working in the future?

quantizor · 2018-10-01T02:16:40Z

Sure thing, I'll add a test

analysis-bot

Code analysis results:

eslint found some issues.
eslint found some issues.
eslint found some issues.

analysis-bot · 2018-10-01T02:42:56Z

jest/__mocks__/FakeComponent.js

@@ -0,0 +1 @@
+export default () => null


semi: Missing semicolon.

analysis-bot · 2018-10-01T02:42:57Z

jest/__tests__/mockComponent-test.js

@@ -0,0 +1,13 @@
+const mockComponent = require('../mockComponent');
+const FakeComponent = require('FakeComponent')


semi: Missing semicolon.

analysis-bot · 2018-10-01T02:42:57Z

jest/__tests__/mockComponent-test.js

@@ -0,0 +1,13 @@
+const mockComponent = require('../mockComponent');
+const FakeComponent = require('FakeComponent')
+const TextInput = require('TextInput')


semi: Missing semicolon.

quantizor · 2018-10-01T02:45:44Z

Side question, why was this done? 76eebce#diff-a562e4a7ac9e900c6d2bc644453e9152

The displayName was removed from a bunch of components which is kind of a mistake because it makes them hard to identify in devtools and react warnings.

elicwhite · 2018-10-01T04:10:00Z

Our understanding is that React.forwardRef was updated to utilize the displayName of the passed in component so those lines were no longer necessary.

quantizor · 2018-10-01T04:14:48Z

It doesn’t as far as I know. It just respects if you set a displayName on the forwardRef

…

On Sun, Sep 30, 2018 at 11:12 PM Eli White ***@***.***> wrote: Our understanding is that React.forwardRef was updated to utilize the displayName of the passed in component so those lines were no longer necessary. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#21429 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAiy1gz4YZRe0Nai_Qcsl6j-2GTODP8fks5ugZY5gaJpZM4XBJes> .

elicwhite · 2018-10-01T04:25:56Z

Hmm. I’m not on a computer to test it out but the docs for forwardRef at least seem to indicate that it has some special behavior for displayName. https://reactjs.org/docs/forwarding-refs.html

quantizor · 2018-10-01T04:39:01Z

@TheSavior The behavior is if you set a displayName on the ForwardRef wrapper it'll take it into account in devtools I believe. forwardRef doesn't know what component it's rendering until render is called so there's no way to acquire the underlying component's displayName statically.

pull-bot · 2018-10-01T04:40:14Z

	Warnings
⚠️	🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.
⚠️	❗ Big PR - This PR is extremely unlikely to get reviewed because it touches 1411 lines.

Generated by 🚫 dangerJS

elicwhite · 2018-10-01T19:58:46Z

View and Text both use forwardRef and don't explicitly set a displayName. Using React DevTools with our current production app, I see the proper name in forwardRef:

Is this inconsistent with what you are seeing?

quantizor · 2018-10-01T20:45:16Z

@TheSavior I think devtools is kind of a special case since it literally executes the render function of the forwardRef to obtain the underlying component and then shows the displayName as a derivation of that. Enzyme and other tooling likely will not go to that length since the fact that forwardRef has a function called "render" is an implementation detail and not part of the public API.

I actually wrote the change that was recently merged into react core to start directly honoring displayName set on forwardRef and this is an extension of that effort.

elicwhite · 2018-10-01T22:16:15Z

Okay, so to make sure I understand, is your request that we add back the annotations to all the components that use forwardRef so that tooling like enzyme is able to read those names?

It seems like that will be hard to enforce everywhere forwardRef is used and is a workaround that adds runtime code to help make build and test time tooling easier. Am I misunderstanding something?

quantizor · 2018-10-02T00:03:27Z

I’d say that’s phase 2 of this PR/changeset, yes. I’m not sure what other recourse there is without having React core do displayName as a getter for forwardRef and emulating the same behavior as devtools because most testing output becomes less useful without the names.

…

On Mon, Oct 1, 2018 at 5:19 PM Eli White ***@***.***> wrote: Okay, so to make sure I understand, is your request that we add back the annotations to all the components that use forwardRef so that tooling like enzyme is able to read those names? It seems like that will be hard to enforce everywhere forwardRef is used and is a workaround that adds runtime code to help make build and test time tooling easier. Am I misunderstanding something? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#21429 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAiy1nhEQNUFboHZwipSlQJEC6BMUcu0ks5ugpUHgaJpZM4XBJes> .

quantizor · 2018-10-02T00:10:12Z

And actually that wouldn’t even work consistently because it’s entirely possible to dynamically render different things inside the forwardRef render based on props.

…

On Mon, Oct 1, 2018 at 7:03 PM Evan Jacobs ***@***.***> wrote: I’d say that’s phase 2 of this PR/changeset, yes. I’m not sure what other recourse there is without having React core do displayName as a getter for forwardRef and emulating the same behavior as devtools because most testing output becomes less useful without the names. On Mon, Oct 1, 2018 at 5:19 PM Eli White ***@***.***> wrote: > Okay, so to make sure I understand, is your request that we add back the > annotations to all the components that use forwardRef so that tooling > like enzyme is able to read those names? > > It seems like that will be hard to enforce everywhere forwardRef is used > and is a workaround that adds runtime code to help make build and test time > tooling easier. Am I misunderstanding something? > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#21429 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAiy1nhEQNUFboHZwipSlQJEC6BMUcu0ks5ugpUHgaJpZM4XBJes> > . >

cpojer · 2019-01-22T14:02:56Z

I'm gonna close this as there hasn't been progress in many months and the conclusion seems to be that this isn't what we want to do.

pass through the displayName from the mocked component

0778d81

without this, most tooling will just say "Component" instead of "Text", "View" etc.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 30, 2018

react-native-bot added Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. ✅Release Notes and removed Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. labels Sep 30, 2018

add tests

1087640

analysis-bot suggested changes Oct 1, 2018

View reviewed changes

add semis

b3100a7

acquire displayName from forwardRef-wrapped components as well

e732675

quantizor requested a review from hramos as a code owner October 1, 2018 04:37

react-native-bot added the ‼️Large PR label Oct 1, 2018

cpojer closed this Jan 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pass through the displayName from the mocked component #21429

pass through the displayName from the mocked component #21429

quantizor commented Sep 30, 2018 •

edited

Loading

elicwhite commented Oct 1, 2018

quantizor commented Oct 1, 2018

analysis-bot left a comment

analysis-bot Oct 1, 2018

analysis-bot Oct 1, 2018

analysis-bot Oct 1, 2018

quantizor commented Oct 1, 2018

elicwhite commented Oct 1, 2018

quantizor commented Oct 1, 2018 via email

elicwhite commented Oct 1, 2018

quantizor commented Oct 1, 2018

pull-bot commented Oct 1, 2018

elicwhite commented Oct 1, 2018

quantizor commented Oct 1, 2018

elicwhite commented Oct 1, 2018

quantizor commented Oct 2, 2018 via email

quantizor commented Oct 2, 2018 via email

cpojer commented Jan 22, 2019

		@@ -0,0 +1,13 @@
		const mockComponent = require('../mockComponent');
		const FakeComponent = require('FakeComponent')

pass through the displayName from the mocked component #21429

pass through the displayName from the mocked component #21429

Conversation

quantizor commented Sep 30, 2018 • edited Loading

Test Plan:

Release Notes:

elicwhite commented Oct 1, 2018

quantizor commented Oct 1, 2018

analysis-bot left a comment

Choose a reason for hiding this comment

analysis-bot Oct 1, 2018

Choose a reason for hiding this comment

analysis-bot Oct 1, 2018

Choose a reason for hiding this comment

analysis-bot Oct 1, 2018

Choose a reason for hiding this comment

quantizor commented Oct 1, 2018

elicwhite commented Oct 1, 2018

quantizor commented Oct 1, 2018 via email

elicwhite commented Oct 1, 2018

quantizor commented Oct 1, 2018

pull-bot commented Oct 1, 2018

elicwhite commented Oct 1, 2018

quantizor commented Oct 1, 2018

elicwhite commented Oct 1, 2018

quantizor commented Oct 2, 2018 via email

quantizor commented Oct 2, 2018 via email

cpojer commented Jan 22, 2019

quantizor commented Sep 30, 2018 •

edited

Loading