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

Error: Invariant Violation: ReactMount: Two valid but unequal nodes with the same data-reactid #1263

Closed
dlindahl opened this issue Mar 17, 2014 · 23 comments
Assignees

Comments

@dlindahl
Copy link

TL;DR fix Load `polymer.js first


Probably related to #1107

When a select or input[type="checkbox"] is rendered inside an unwrapped Polymer element's child node, the following error is thrown:

Error: Invariant Violation: ReactMount: Two valid but unequal nodes with the same `data-reactid`

I would imagine that radio buttons, etc. would also throw.

I am not sure if this is an actual bug or a limitation of React and the ShadowDOM/Polymer. But the error message is... not exactly helpful in determining what to do.

I wrote a simple JSBin that demonstrates the problem: http://jsbin.com/pinijoho/2

@dlindahl
Copy link
Author

After some additional toying with Polymer + React, I ran into the same bug with a more straightforward example: http://jsbin.com/xoxip/2/

In this JSBin, the React component simply toggles a CSS class when the state changes due to a mouse click.

The CSS class is added properly on the first click, but when toggled off, removing the CSS class throws the same Two valid but unequal error.

@benjamn benjamn self-assigned this Mar 17, 2014
@benjamn benjamn added the bug label Mar 17, 2014
@benjamn
Copy link
Contributor

benjamn commented Mar 17, 2014

There's definitely something weird going on here. The error message is as accurate as it can be, though it's generally difficult to guess (at the framework level) what the root source of the problem was.

When we've seen errors like this in the past, it was typically because .cloneNode() was being used by client code to make copies of React-generated nodes, also copying the data-reactid attribute (which is supposed to be unique), but my brief investigation doesn't point there this time.

@zpao
Copy link
Member

zpao commented Mar 17, 2014

I actually get a different error in Firefox as it's trying to get the node for an event. Can repro in Chrome though.

TypeError: Argument 1 of Node.contains does not implement interface Node.
Stack trace:
containsNode@http://fb.me/react-0.9.0.js:14157:5
...

@dlindahl
Copy link
Author

Let me rephrase, I am sure that the error is as explicit as it can be. But for someone that is not intimately familiar with what React is trying to do, its like running into a brick wall. 😄

But that is besides the point, the issue isn't with the error messaging.

It sounds like this may be an issue with Polymer's ShadowDOM polyfill. I tried playing with it a bit more and I believe I was able to get it to work by not unwrap-ing the Light DOM node that the React component is rendered into:

http://jsbin.com/xoxip/3/

It sounds like Polymer's wrap function wraps the DOM Api with its own methods for interacting with the Shadow- and LightDOM. Since React is using appendChild somewhere, and I was unwrap-ing the root element, it sounds like Polymer and React disagreed about what the tree should look like. Removing the explicit unwrap exposes Polymer's version of appendChild to React and then it starts playing nicely again.

The curious thing is why I felt it necessary to add the unwrap in the first place... I am pretty sure that React was complaining about the Light DOM node not being of the right NODE_TYPE, but I made a couple changes to my rendering loop in the meantime which may have eliminated that issue...

Thank you to both of you for looking into this. Since the JSBin seems to be working now as well as my application, I am going to go ahead and close this and hope that we've all learned something new.

@benjamn
Copy link
Contributor

benjamn commented Mar 17, 2014

That story is consistent with my findings: the invariant in ReactMount.getID was failing because cached !== node, where cached was a <span> and node was something like { impl: cached, ...}, so it sounds like the node was one of the ShadowDOM wrappers you're talking about.

@arv
Copy link

arv commented Mar 17, 2014

When using polymer's shadow dom polyfill you should never see any raw DOM nodes. If you do it is either a bug or one of the known limitations. It would be useful to know how you got a reference to your node?

@benjamn
Copy link
Contributor

benjamn commented Mar 17, 2014

@arv React creates most of its nodes by setting .innerHTML, so unless Polymer is hooking that setter, that might be one way raw DOM nodes get exposed?

@arv
Copy link

arv commented Mar 17, 2014

We do wrap innerHTML. Accessors and the buggy implementation of them in WebKit and Blink is the reason why we need to wrap everything.

Known limitations are the accessors of

@arv
Copy link

arv commented Mar 17, 2014

...of window and document since these are non configurable in webkit.

@dlindahl
Copy link
Author

@arv I was indeed getting wrapped nodes, but I ran into an issue when rendering a React component inside one of them. My solution at the time was to unwrap the polyfilled node reference and use that with React. That seemed to fix the problem initially, but then I ran into this issue.

Turns out that passing the unwrapped node to React was a bad idea and it works fine without doing that. I think the root cause of my initial issue was due to a misuse of the React rendering API. I believe I was calling renderComponent for each upstream update instead of using setProps.

So all in all, everything actually works just fine 😄

Now if I can only figure out if its possible to use React to render Polymer elements...

@arv
Copy link

arv commented Mar 18, 2014

That should be easy. Polymer elements are just custom elements so createElement, innerHTML etc should just work.

@dlindahl
Copy link
Author

I hate to do this, but I just ran into the reason I had added unwrap to my rendering loop.

When rendering a React component inside a Polymer element, using a Light DOM node as the root node for the React component works fine until you need to conditionally render a child node (An Error: Assertion failed error is thrown)

You can get around this by passing React an unwrapped node with unwrap. But by doing so, you lose the ability to toggle CSS classes or interact with form elements (An Invariant Violation: ReactMount: Two valid but unequal nodes with the same 'data-reactid' error is thrown)

Unfortunately, I am having trouble reproducing the exact behavior in JSBin.

Before I spend too much more time, does anyone think that this is a losing battle? Is there any reason to assume that React is compatible with Polymer and vice versa?

@dlindahl dlindahl reopened this Mar 18, 2014
@dlindahl
Copy link
Author

Ok, I've created the mother of all test cases: http://jsbin.com/qoquv/1/

This creates four React components:

  • Regular form
  • CSS class toggler
  • Table row append/remove-er
  • List item append/remove-er

It then renders each of those components in the following ways:

  • Inside a Polymer element with an unwrapped Light DOM node
  • Inside a Polymer element with a wrapped Light DOM node
  • Inside a plain DOM element

Each variety seems to exhibit different behavior.

Items of note:

  • The class toggler that fails to respond when passed a wrapped DOM node. Maybe its because the Polymer polyfill does things with events? Not sure, but oddly enough, I don't seem to see this in my own non-trivial application...
  • The pure React + DOM renderings fail to work properly when the Polymer polyfill is included on the page. Commenting out the Polymer polyfill, framework, and custom element definitions allow the pure React components to work as expected. I imagine this is due to the polyfill wrapping all DOM nodes, whether you want them too or not. Using unwrap in this instance causes React to throw the Two valid but unequal error.

I'm not sure where to go from here. As I mentioned before, it is unclear to me if React should work more to be compatible with Polymer, if Polymer should work more to be compatible with React, if this is just a really bad idea that I should just drop, or wait for Polymer to mature.

It would also be interesting to see if Mozilla's XTag polyfill exhibits the same behavior, but I'm already months late on a project and do not have the time to investigate that path. Perhaps @vjeux has some insight based on his blog post

@arv
Copy link

arv commented Mar 18, 2014

React should not need to do anything. This is all in Polymer's court. I'll take a look next week when I'm back from vacation.

@dlindahl
Copy link
Author

@arv That is fantastic, thanks so much. Should I reopen this issue at Polymer/polymer?

@vjeux
Copy link
Contributor

vjeux commented Mar 21, 2014

I just prototyped using x-tag during an afternoon and it seemed to work but I didn't thoroughly test it.

In any case, to your question if React/Polymer should inter-operate, definitively yes. We should make it as easy as possible to embed React component inside of Polymer components and vis-versa.

I don't have enough context on this actual bug, but if we can make the internals of React more polymer friendly we'd generally be happy to do so.

@dlindahl
Copy link
Author

@vjeux I think the problem is actually with Polymer. After creating the JSFiddle mentioned earlier, it looks like the mere presence of the Polymer polyfill prevents React from working on any element.

@arv mentioned that the issue "...is all in Polymer's court" so I will assume he will take the lead on further investigation. So I guess you guys can just stand by until the Polymer team has a chance to look into it further.

Thanks for taking a look 🍻

@arv
Copy link

arv commented Mar 24, 2014

@dlindahl I got it working by:

  1. Using 0.2.1 of Polymer.
  2. Making sure polymer/platform.js comes before react to ensure a consistent view of the wrappers in the page.
  3. Remove all manual unwrapping.

http://jsbin.com/poqoxija/1/

@dlindahl
Copy link
Author

@arv 💖 This is fantastic.

Is it worth documenting this order dependency in either of the two projects? Is there additional work that can be added to either project to remove the dependency?

@arv
Copy link

arv commented Mar 26, 2014

Polymer documents this at

http://www.polymer-project.org/docs/start/platform.html

Note: Due to the nature of some of the polyfills, to maximize compatibility with other libraries, make sure that platform.js is the first script tag in your document’s <head>.

@petehunt
Copy link
Contributor

Are there other polyfills that would cause this problem or just Polymer? I'm not aware of any others but maybe they're out there.

If there are others I'd say we should update the error message to include a note about polyfills. If there aren't I think that it's probably Polymer's job to message this.

@arv
Copy link

arv commented Mar 26, 2014

I think the onus is on us (Polymer). Most polyfills do not need to control
the whole world.
On Mar 25, 2014 8:18 PM, "Pete Hunt" notifications@github.com wrote:

Are there other polyfills that would cause this problem or just Polymer?
I'm not aware of any others but maybe they're out there.

If there are others I'd say we should update the error message to include
a note about polyfills. If there aren't I think that it's probably
Polymer's job to message this.


Reply to this email directly or view it on GitHubhttps://github.com//issues/1263#issuecomment-38637396
.

@dlindahl
Copy link
Author

@arv Since the errors caused by not including platform.js first can be dramatic and confusing, could you make the note in the Polymer docs a bit more assertive?

Since this is no longer an issue, I'm closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants