Skip to content

Conversation

@jimfb
Copy link
Contributor

@jimfb jimfb commented Jul 23, 2015

Added note about multiple Reacts, since this error commonly implies that.

@jimfb
Copy link
Contributor Author

jimfb commented Jul 23, 2015

@spicyj you have context on this diff. For those following along at home, we've seen a bunch of github issues where people are confused, which means it's probably confusing a bunch of people who don't report it to us. We want to point users in the right direction with a helpful/timely message.

Lint failure is from a different file, unrelated to change. Should be fixed independently.

@zpao
Copy link
Member

zpao commented Jul 23, 2015

(lint is some false positive thanks to build caching, cleared and retriggered - I hit it on master too)

@maoziliang
Copy link

How about add a magic token in the global object. Use the token to determine whether a past React had been loaded. And then print a warning ?

@jimfb
Copy link
Contributor Author

jimfb commented Jul 25, 2015

@maoziliang We tried that, but it ended up warning people in unit tests and we had to revert the diff (#3646).

@zpao or @spicyj ready to merge?

@jimfb
Copy link
Contributor Author

jimfb commented Jul 27, 2015

Ping.

@jimfb
Copy link
Contributor Author

jimfb commented Aug 4, 2015

@zpao or @spicyj Is there a reason we are not moving this forward, or should I keep pinging the thread? Soon I'll get as good at this as pingbot!

@sophiebits
Copy link
Collaborator

I want to criticize the too-long wording but haven't gotten around to it yet…

@jimfb
Copy link
Contributor Author

jimfb commented Aug 5, 2015

@spicyj Comment overall or specifically my addition?

@sophiebits
Copy link
Collaborator

Mostly it's just really long now.

@syranide
Copy link
Contributor

syranide commented Aug 5, 2015

@jimfb An idea would be to add a property (not attribute) to React root nodes with a per-React instance random value. Then for all the various asserts that can be affected by multiple Reacts we first check whether the property of the root node of that React tree has the value we expect, if it doesn't we know there's a different instance present.

@jimfb jimfb force-pushed the ref-error-means-multiple-react branch from 1545575 to 4e6a1eb Compare August 11, 2015 19:41
@jimfb
Copy link
Contributor Author

jimfb commented Aug 11, 2015

@spicyj Better? It is now 4 lines instead of 7 lines :P.

@syranide That's not a bad idea. If the root node is a web component, it might respond badly to having a random property set on it, but maybe we skip the check for web components? It's worth looking into, but not worth blocking this fix. This is a trivial mitigation that will catch the most common case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

https, period at the end of the sentence

@sophiebits
Copy link
Collaborator

Okay, I guess this is fine. Can you shorten the remove message too? (Not sure I've ever seen it though since you'd always hit the add invariant first in practice.)

@jimfb jimfb force-pushed the ref-error-means-multiple-react branch from 4e6a1eb to 0597654 Compare August 11, 2015 19:59
@jimfb jimfb force-pushed the ref-error-means-multiple-react branch from 0597654 to e984d2b Compare August 11, 2015 20:00
@jimfb
Copy link
Contributor Author

jimfb commented Aug 11, 2015

@spicyj Yeah, done. Is it even possible to hit that remove path? Seems like you'd need to add the ref in order to remove it, so it would be impossible to hit.

jimfb added a commit that referenced this pull request Aug 11, 2015
Added note about multiple Reacts, since this error commonly implies that.
@jimfb jimfb merged commit 079a6b0 into facebook:master Aug 11, 2015
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.

6 participants