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

Running two copies of React on a page can give errors because both use ID '.0' #1939

Closed
sophiebits opened this issue Jul 26, 2014 · 32 comments · Fixed by #4747
Closed

Running two copies of React on a page can give errors because both use ID '.0' #1939

sophiebits opened this issue Jul 26, 2014 · 32 comments · Fixed by #4747

Comments

@sophiebits
Copy link
Collaborator

Maybe we can do something smarter with the root index generation on the client side? Seems like this should work.

dmnd added a commit to dmnd/atom that referenced this issue Jul 27, 2014
dmnd added a commit to dmnd/git-blame that referenced this issue Jul 27, 2014
This will make it much easier to update the view after it becomes visible. This
commit is just a rewrite though and I haven't added any functionality yet.

This commit is dependent on atom/atom#3102 which allows me to workaround
facebook/react#1939. I might vendorize a hacked version of React if those bugs
take a while to be fixed.

Also I noticed one regression - clicking in the editor seems to have a slight
delay when the gutter is open. I'm not sure what's causing this but will try
to fix.

Test plan:

I tested every piece of functionality I could think of, including:

  * Clicking on hash links
  * Scrolling
  * Opening when already scrolled
  * Adjusting width
  * Opening on a file that's not checked in (error still appears)
@chenglou
Copy link
Contributor

Use the version number as a prefix could work? (You wouldn't have the same version of React twice, or else you didn't dedupe correctly).

@syranide
Copy link
Contributor

This could solve itself when we drop IDs from the DOM (and be somewhat supported). As we would use an internal Map for translating node to instance (and the fallback property could be randomly generated). If we run with the data-react marker compromise and want to support multiple versions, data-react would also have to randomly generated.

However, for multiple versions to really play nice, we would probably (optionally) need to attach event listeners to the root nodes instead of the document (as I suggested for playing nice with other frameworks too).

@nathansobo
Copy link

We're planning to implement the Atom workspace views in React, so that packages can provide React components as panels and pane items.

Ideally, different packages could rely on their own independent versions of Atom so packages wouldn't break if we upgraded React in core. Am I correct in thinking that might be a little unrealistic? I don't imagine you guys plan on supporting interoperation between components written in different versions of React (thought that would be awesome).

So instead our plan is to export the instance of React we use in core for use by package authors in building their own views, so everything interoperates. The downside of this is that upgrading React in core could break packages. So we may need to supply a shim to old packages when that happens to ease the transition. Hopefully that won't be too difficult.

Just thought I'd share our use case, in case it shapes any design decisions as you change the API moving forward.

@sophiebits
Copy link
Collaborator Author

@nathansobo Atom plugins was what prompted this issue. :) We're not going to support using multiple versions of React seamlessly together in the near term, though it's possible it'll work in the far future. Until then, your best approach may be to have plugins populate iframes with plugin contents so that the scripts run in an isolated window.

@nathansobo
Copy link

I think we'll just give everyone a reference to the same React instance. The iframe approach is unfortunately too isolated for the kinds of things people are doing in packages. Hopefully we can shim API changes. Are you guys planning on a bunch for 1.0?

@syranide
Copy link
Contributor

Multiple versions of React cooperating is technically not very advanced (#1939 (comment)). I think the biggest issue for now is that events handlers are always bound to the document, so events would not be playing nice at all.

Shimming is probably not going to be a viable strategy unless you're referring to maintaining a separate branch, but even that could probably turn into a major headache with some changes to come.

@nathansobo One possibility is to maintain separate Atom-branches of all major React releases, which does play well with multiple versions, it should generally not be that complex I think. It would probably be the more realistic approach, if any, I think. Although targeting different JSX versions is an interesting problem if that applies to you.

@nathansobo
Copy link

@syranide Interesting. Do you mean multiple numeric versions of React or multiple instances? I'm talking about a component from version 0.10 being embedded in a component tree from version 0.11. If that's something that could be possible, that would be amazing.

@syranide
Copy link
Contributor

@nathansobo Multiple numeric versions of React, or any library for that matter. If I'm not mistaken, there are only two parts of React DOM that currently prevents this, the event system and DOM reactids.

  1. Fixing DOM reactids is simple, the simplest solution being just suffixing ID_ATTR_NAME with a random/unique string on startup. Each React version sees only the DOM that is managed by itself.
  2. The event system is a little more complicated, the problem is that all event listeners are bound to document, this does not play nice with other libraries (and event handlers). I've proposed moving event listeners to each mount root, which should be preferable as it would then also play nice with other event handlers/libraries. This is theoretically very simple, but does require a few lines of code so that the set of listeners is tracked per mount root instead and to prevent an event from being acted on multiple times by the same instance.

So, unless I missed something, that should be all that is needed to support multiple numeric versions of React. It does require maintaining a separate "Atom-compatible branch of each React version", but it should be possible to just rebase the fix onto each new version of React, without complications.

I don't know what the devs feel about # 2, but these are all rather simple fixes that I don't see why we wouldn't want to apply (one way or another) in the future.

But as I mentioned, I don't know if/how you use JSX today, but that is something that would also have to be available in a few different versions. There was one "breaking" JSX update a while back and it seems like there will be one or two more in the future.

@nathansobo
Copy link

Sorry, one more clarifying question. What you're saying makes sense with regard to multiple root components coexisting, but I'm still not clear on the scenario in which a parent composite component from one version of React owns a child component from another version.

Say for example I have implemented Atom's workspace in React and I also want to implement Atom's tree-view package in React. I was imagining that the tree-view component would be referred to directly in the workspace's render method and end up getting owned by the workspace. Isn't the contract between these components a bit more involved than just the React id's? What am I missing here?

@leoasis
Copy link

leoasis commented Jul 30, 2014

Regarding react ids with random suffixes, won't that break the reconciliation with server side pre-rendered react markup? Haven't looked at the internals of this, just wanted to make sure.

@syranide
Copy link
Contributor

@nathansobo Fire away :)

I don't really have the full picture of what you're intending to do (so don't take my word for anything). As for my idea, one would have to use a "bridge" for rendering component of other versions, this could simply be a custom (generic) component that calls ReactVersionXYZ.renderComponent (this is roughly equivalent to how render() works internally).

Just something to visualize what I mean: (I assume that required React version is stored statically on the component class to be rendered)

render: function() {
  return (
    <div>
      <ReactBridge type={MyCompThatIsAnotherVersion} propA={1} propB={test'} />
    </div>
  );
}

So this would allow composition of components of different React versions (you own the instance of the bridge, not the actual component), but where the meat of each component should preferably depend on the same React version, everything should behave as you would expect (updates, callbacks, etc). However descriptors are version specific, so you couldn't pass unwrapped component classes or descriptors through the bridge and expect it to work. That is, the props you provide the component cannot be React-specific in nature.

Depending on how tightly you're integrating React, this could be a significant issue as batching wouldn't happen (by default) across such bridges/boundaries. It's definitely imagineable that one could rip out the virtual DOM/transactions out of React and have all versions of React cooperate, but I don't know if that is practical.

It most certainly is not as neat as using a single React version from top to bottom, but I think the merit of something like this is that each implementation ultimately becomes independent of whatever is on the other side. So when React 2.0 comes around, modules would simply adopt new versions as they please, rather than be abandoned.

But it also sounds like you might be integrating quite deeply with React and never being able to improve your API for React (as you would have to support each React version indefinitely) could be equally horrible.

Perhaps this is not at all applicable to what you have mind (if what I said makes sense :)), but you guys have an interesting problem at your hands, and I think a lot of it comes down to how long (or how far) you're willing to support "old addons".

@ryanseddon
Copy link
Contributor

So this was brought up in react-frame-component when trying to use it in a Chrome extension on a page that already used react.

The solution was to do the following:

require('react/lib/DOMProperty').ID_ATTRIBUTE_NAME = 'data-myproductid';

cc @nelix

@danvk
Copy link

danvk commented May 8, 2015

@ryanseddon this solution no longer seems to work. It does change the attribute, but I get the following error:

Uncaught Error: Invariant Violation: findComponentRoot(..., .0.0.2): Unable to find element. This probably means the DOM was unexpectedly mutated (e.g., by the browser), usually due to forgetting a <tbody> when using tables, nesting tags like <form>, <p>, or <a>, or using non-SVG elements in an <svg> parent. Try inspecting the child nodes of the element with React ID.

I'd love a workaround for this—it makes it impossible to distribute a JS library which uses React.

@goofballLogic
Copy link

+1 this is a critical use case in a company i am working with.

@ryanseddon
Copy link
Contributor

I just tried this out, you need to update the attribute before you require react and it doesn't throw and works as expected.

require('react/lib/DOMProperty').ID_ATTRIBUTE_NAME = 'data-myproductid';
var React = require('react');

@danvk
Copy link

danvk commented May 13, 2015

@ryanseddon 👍 that did the trick. Note that you really have to make sure to do the monkey-patching before any of your code does require('react'), or else things will break. The first place may not be where you expect.

I wound up making a little shim:

// react-shim.js
// See https://github.com/facebook/react/issues/1939
require('react/lib/DOMProperty').ID_ATTRIBUTE_NAME = 'data-myid';
var react = require('react/addons');
module.exports = react;

and replaced all require('react') statements with require('./react-shim').

@charltoons
Copy link

@ryanseddon 's solution works if you are looking to create multiple top level entry points, basically multiple React.render()s. My use case was a pastiche site where some sections were written in React, some were another legacy framework. I wanted each section to be modularly responsible for its own React.render so that they could be removed or swapped around at will. So far, its working well, even the event system.

@dvdplm
Copy link

dvdplm commented Aug 19, 2015

Maybe I'm thick, but it seems like @ryanseddon's solution to this requires using requirejs and a non-production (non-minified) version of react? What if I load react from https://cdnjs.cloudflare.com/ajax/libs/react/0.13.3/react.min.js, how do I go about poking in the bowels of DOMProperty? Is that even possible?

Given how many of us there are experiencing pain caused by this, and the relative difficulty of a definitive fix, would a PR exposing a way to set the id attribute name be welcome? Something along the lines of React.setIdAttributeName("flowers-and-peace");

@jeberly
Copy link

jeberly commented Aug 19, 2015

+1 to @dvdplm

@dvdplm
Copy link

dvdplm commented Aug 20, 2015

I added two PRs (one from 0.13.3, the other off current tip) to make the ID_ATTRIBUTE_NAME configurable with React.setIdAttributeName("data-my-own-private-reactid");. This must be called before any rendering occurs of course.

PR for 0.13.3: #4668
PR for master: #4669

@dlopuch
Copy link

dlopuch commented Aug 27, 2015

+1

Major vendors are now starting to package embeddable widgets built on React. For example, we've encountered the Invariant Violation: ReactMount: Two valid but unequal nodes with the same 'data-reactid' error using Zendesk's embeddable widget -- their React packaging's DOM was interfering with our React packaging's DOM (via an event handler, if that helps).

It's a completely valid use-case to package your own distro of React if you're building a browser extension or an embeddable widget imho. With more and more adoption of React, though, there should be an easy React.setIdAttributeName() so different vendors can play nice with each other on the same site without monkeypatching a custom build.

@dvdplm
Copy link

dvdplm commented Aug 27, 2015

@dlopuch that is very similar to our situation and I agree wholeheartedly. :)

@sophiebits
Copy link
Collaborator Author

I understand this is a problem. Are your DOM trees nested within each other or completely separate?

@dlopuch
Copy link

dlopuch commented Aug 27, 2015

Separate in my embeddable widget case (these things often ask you to put a script in your head which bootstraps their own app and injects a new div into your body).

The ID_ATTRIBUTE_NAME workaround fixed it, but it should be easier to do with some docs telling embeddable and extension developers how/why to use it when bundling their own copy of react.

@sophiebits
Copy link
Collaborator Author

Off the top of my head I'm not sure how that causes problems. Do you know when that error is triggered? (In response to some event handler or something else?)

@dlopuch
Copy link

dlopuch commented Aug 27, 2015

Happens in response to a mouseover.

We have two React's on our site -- the embedded widget's react and our react. There are two elements that have data-reactid=".2" in the DOM, one created by the embedded widget's react and one created by our react.

The full stacktrace is:

Error: Invariant Violation: ReactMount: Two valid but unequal nodes with the same `data-reactid`: .2
    at invariant (https://local.datahero.com/js/lib/react-with-addons-0.13.3.js:20302:15)
    at Object.getID (https://local.datahero.com/js/lib/react-with-addons-0.13.3.js:12433:43)
    at Object.ReactMount.isRenderedByReact (https://local.datahero.com/js/lib/react-with-addons-0.13.3.js:13024:25)
    at ReactMount.getFirstReactDOM (https://local.datahero.com/js/lib/react-with-addons-0.13.3.js:13039:22)
    at Object.EnterLeaveEventPlugin.extractEvents (https://local.datahero.com/js/lib/react-with-addons-0.13.3.js:2442:9)
    at Object.EventPluginHub.extractEvents (https://local.datahero.com/js/lib/react-with-addons-0.13.3.js:2863:46)
    at Object.ReactEventEmitterMixin.handleTopLevel [as _handleTopLevel] (https://local.datahero.com/js/lib/react-with-addons-0.13.3.js:11236:33)
    at handleTopLevelImpl (https://local.datahero.com/js/lib/react-with-addons-0.13.3.js:11329:24)
    at ReactDefaultBatchingStrategyTransaction.Mixin.perform (https://local.datahero.com/js/lib/react-with-addons-0.13.3.js:18402:20)
    at Object.ReactDefaultBatchingStrategy.batchedUpdates (https://local.datahero.com/js/lib/react-with-addons-0.13.3.js:9669:19)

Error happens when we mouseover on the embedded widget's element. react-with-addons-0.13.3.js:12433:43 (the getID() function) is a bit weird -- it's in our React's context, cached resolves to our reactid=.2 element, but node is a node from the other React, the one we're mousing over.

If it matters, the other React's node that we're mousing over is an iframe.

Hope this helps.

@ryanseddon
Copy link
Contributor

@dlopuch this is a small world, I'm the tech lead on the Zendesk widget.

I created a test case reproducing the error. It definitely seems related to the fact that our widget creates an iframe and shares a data-reactid with the host page. We'd love to use the exposed api to namespace the data attribute react uses.

sophiebits added a commit to sophiebits/react that referenced this issue Aug 31, 2015
Should fix facebook#1939.

Test Plan:
With two copies of React, render a div using React1 and use that as a container to render a div with React2. Add onMouseEnter/onMouseLeave to both divs that log. Mouse around and see correct logs (as if each React was isolated), no errors.
sophiebits added a commit to sophiebits/react that referenced this issue Sep 1, 2015
Should fix facebook#1939.

Test Plan:
With two copies of React, render a div using React1 and use that as a container to render a div with React2. Add onMouseEnter/onMouseLeave to both divs that log. Mouse around and see correct logs (as if each React was isolated), no errors.
@Halama
Copy link

Halama commented Sep 7, 2015

I have exactly the same issue with Zendesk widget. Thanks for suggesting ID_ATTRIBUTE_NAME trick! it worked for me.

I wasn't able to make it work with React Hot loader because it requires react at the beginning. So I inject it only for production https://github.com/keboola/kbc-ui/commit/756b9ddd1c820d1ca3ee08b8143922cd9db1d7c3

sophiebits added a commit to sophiebits/react that referenced this issue Sep 9, 2015
Should fix facebook#1939.

Test Plan:
With two copies of React, render a div using React1 and use that as a container to render a div with React2. Add onMouseEnter/onMouseLeave to both divs that log. Mouse around and see correct logs (as if each React was isolated), no errors.
sophiebits added a commit to sophiebits/react that referenced this issue Sep 9, 2015
Should fix facebook#1939.

Test Plan:
With two copies of React, render a div using React1 and use that as a container to render a div with React2. Add onMouseEnter/onMouseLeave to both divs that log. Mouse around and see correct logs (as if each React was isolated), no errors.
sophiebits added a commit that referenced this issue Sep 10, 2015
Don't crash in event handling when mixing React copies
@gisenberg
Copy link

Hey folks,

Hate to resurrect an old issue, but the ID_ATTRIBUTE_NAME trick only seems to work in some cases. If you add an onClick to an element in the render method of a React class after using ID_ATTRIBUTE_NAME = 'data-foo', you'll end up with an 'Unable to find element' error at runtime. Directly modifying DOMProperty.js's assignment of ID_ATTRIBUTE_NAME resolves the issue.

This scenario is important because it's impossible to use a shared React component in a large multi-tenant system where different versions of React might already have been included on the page. I'm left wondering if I missed something or if there isn't a better way to solve this, since it seems that the only way to solve it right now is to fork React to change DOMProperty.

@dvdplm
Copy link

dvdplm commented Feb 3, 2016

the only way to solve it right now is to fork React to change DOMProperty.

This is what we are doing and we've had no issues since. It is very far from ideal but so far it has been the only workable solution.

@johanneslumpe
Copy link
Contributor

Still seeing this issue, this time related to the Grammarly plugin. Tried to change ID_ATTRIBUTE_NAME in our build of react, but it didn't help at all. Still getting an invariant error about two valid but unequal nodes. Not sure if this is the correct thread to report this, but I feel it's related. The original bug report around Grammarly was posted here: https://discuss.reactjs.org/t/invariant-failures-due-to-chrome-extensions-that-also-use-react/1765 Furthermore, the team around the Grammarly extension already changed their ID_ATTRIBUTE_NAME, which apparently also didn't help.

@hakunin
Copy link

hakunin commented Mar 31, 2016

@spicyj This issue is just horrible, but so easy to fix as @dvdplm pointed out! I spent all day trying to figure this out, trying to compile React at the moment and its just a time suck!

Update even with custom react build which lets me change the react id, I still have the same issue with Grammarly. Possibly, because they inject their dom into mine.

@johanneslumpe have you found a solution or a workaround to this?

Update 2: Nothing else worked, so I updated to React 15rc2 and tricked my app to think it uses v14 by requiring v 14 in package.js and doing this in webpack:

  resolve: {
    extensions: ['', '.js', '.jsx'],
    alias: {
      lib: path.join(process.cwd(), 'app', 'lib'),
      react: path.resolve('./vendor_modules/react'), // <-- this 
      'react-dom': path.resolve('./vendor_modules/react-dom'), // <-- this 
    },
  },

mikewheaton pushed a commit to microsoft/fluentui that referenced this issue Jul 23, 2016
- Move React to devDep and peerDep. Reference: http://stackoverflow.com/a/30454133/1436671
- Release the React semver restriction to 0.14.x.
- The peer dependency leaves the App to resolve the React dependencies.
- It is a MUST to do so, we cannot guarantee the App is using webpack and leverage dedup.
- If not dedup and two React copies run in the page, it leads problems. Like this: facebook/react#1939
- Follow reading the React new version schema: http://facebook.github.io/react/blog/2016/02/19/new-versioning-scheme.html
mikewheaton pushed a commit to microsoft/fluentui that referenced this issue Jul 23, 2016
- Move React to devDep and peerDep. Reference: http://stackoverflow.com/a/30454133/1436671
- Release the React semver restriction to 0.14.x.
- The peer dependency leaves the App to resolve the React dependencies.
- It is a MUST to do so, we cannot guarantee the App is using webpack and leverage dedup.
- If not dedup and two React copies run in the page, it leads problems. Like this: facebook/react#1939
- Follow reading the React new version schema: http://facebook.github.io/react/blog/2016/02/19/new-versioning-scheme.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.