-
Notifications
You must be signed in to change notification settings - Fork 48.6k
Running two copies of React on a page can give errors because both use ID '.0' #1939
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
Comments
Fixes atom#3101 and works around facebook/react#1939.
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)
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). |
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 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). |
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. |
@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. |
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? |
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. |
@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. |
@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.
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. |
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 |
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. |
@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 Just something to visualize what I mean: (I assume that required React version is stored statically on the component class to be rendered)
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". |
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 |
@ryanseddon this solution no longer seems to work. It does change the attribute, but I get the following error:
I'd love a workaround for this—it makes it impossible to distribute a JS library which uses React. |
+1 this is a critical use case in a company i am working with. |
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'); |
@ryanseddon 👍 that did the trick. Note that you really have to make sure to do the monkey-patching before any of your code does 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 |
…ll be deprecated from npm
@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. |
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 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 |
+1 to @dvdplm |
+1 Major vendors are now starting to package embeddable widgets built on React. For example, we've encountered the 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 |
@dlopuch that is very similar to our situation and I agree wholeheartedly. :) |
I understand this is a problem. Are your DOM trees nested within each other or completely separate? |
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. |
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?) |
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 The full stacktrace is:
Error happens when we mouseover on the embedded widget's element. If it matters, the other React's node that we're mousing over is an iframe. Hope this helps. |
@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 |
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.
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.
I have exactly the same issue with Zendesk widget. Thanks for suggesting I wasn't able to make it work with React Hot loader because it requires |
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.
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.
Don't crash in event handling when mixing React copies
Hey folks, Hate to resurrect an old issue, but the 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 |
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. |
Still seeing this issue, this time related to the Grammarly plugin. Tried to change |
@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:
|
- 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
- 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
Maybe we can do something smarter with the root index generation on the client side? Seems like this should work.
The text was updated successfully, but these errors were encountered: