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

Fix Flow issue and revert to showing "Unknown" in warnings for anonymous components #8863

Merged
merged 3 commits into from Jan 25, 2017

Conversation

edvinerikson
Copy link
Contributor

@edvinerikson edvinerikson commented Jan 25, 2017

For future reference, this fixes mistakes made in #8857 and #8858.

@edvinerikson
Copy link
Contributor Author

I can't really understand why it fails

@gaearon
Copy link
Collaborator

gaearon commented Jan 25, 2017

The CI shows that a previously passing test now fails in Fiber mode:

--- a/scripts/fiber/tests-passing-except-dev.txt
+++ b/scripts/fiber/tests-passing-except-dev.txt
@@ -116,3 +116,4 @@ src/renderers/shared/shared/__tests__/ReactMultiChild-test.js
 
 src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js
 * should warn for childContextTypes on a functional component
+* deduplicates ref warnings based on element or owner
diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt
index 050dd41..189f341 100644
--- a/scripts/fiber/tests-passing.txt
+++ b/scripts/fiber/tests-passing.txt
@@ -1560,7 +1560,6 @@ src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js
 * should throw on string refs in pure functions
 * should warn when given a string ref
 * should warn when given a function ref
-* deduplicates ref warnings based on element or owner
 * should provide a null ref

Read how to run tests in Fiber mode: #7925 (comment).

@gaearon
Copy link
Collaborator

gaearon commented Jan 25, 2017

It's likely failing because two different classes are Unknown in that test, and it gets deduplicated.

@gaearon
Copy link
Collaborator

gaearon commented Jan 25, 2017

Okay, so it turned out we have a bunch of call sites that explicitly check for != null result and have special logic when the name is not available (e.g. don’t print an extra useless message like Check the render method of Unknown component).

So null was useful after all.

@edvinerikson
Copy link
Contributor Author

Yeah I noticed that as well. I will revert to null

@gaearon
Copy link
Collaborator

gaearon commented Jan 25, 2017

Sorry for the churn 😅

@edvinerikson
Copy link
Contributor Author

No worries :D

@gaearon
Copy link
Collaborator

gaearon commented Jan 25, 2017

I don't think this will pass Flow, would it? It’s effectively ?string now.

@edvinerikson
Copy link
Contributor Author

oops.. missed that one..

@edvinerikson
Copy link
Contributor Author

success 🎉

@gaearon gaearon changed the title Component -> Unknown Fix Flow issue and revert to showing "Unknown" in warnings for anonymous components Jan 25, 2017
@gaearon gaearon merged commit 0c7f21a into facebook:master Jan 25, 2017
@edvinerikson edvinerikson deleted the rename-component branch January 25, 2017 14:51
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.

None yet

3 participants