Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove local getComponentName from ReactPartialRenderer and use shared version #11999

Closed
wants to merge 1 commit into from

Conversation

wyze
Copy link
Contributor

@wyze wyze commented Jan 9, 2018

This removes the local getComponentName function from ReactPartialRenderer and uses the one found in shared package.

I noticed this while working on #11993. This was the only other getComponentName function that was defined separately than the shared package.

This removes the local getComponentName function from ReactPartialRenderer and uses the one found in shared package.
@@ -187,7 +181,7 @@ function warnNoop(
if (__DEV__) {
const constructor = publicInstance.constructor;
const componentName =
(constructor && getComponentName(constructor)) || 'ReactClass';
(constructor && getComponentName({type: constructor})) || 'ReactClass';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part is kinda hacky and is the reason they weren’t shared. It’s just a different data structure. Maybe it’s reasonable to change the function itself to accept the type directly, but I would rather have duplication than “lie” about the type like here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I had the hacky reservation while making the change. I’m open to changing the shared function to just accept the type or I can close this PR. Doesn’t bother me either way, just let me know how you want to proceed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s just keep it how it is? I don’t see us changing this function anyway so it’s not worth unifying.

@wyze wyze closed this Jan 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants