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

Warn when multiple instances of React are loaded on the same page #3580

Conversation

robertknight
Copy link
Contributor

This causes a variety of hard-to-debug issues. This implements the proposed warning from the comments in #2402.

There are already a number of specific places in the code which warn about using multiple copies of React but those don't always get triggered. This adds a check at the top of the main module which spits out a warning before any of the initialization runs.

Fixes #2402

(Apologies, I closed the previous PR due to a force push in my branch)

This causes a variety of hard-to-debug issues.
See facebook#2402 for examples.

Fixes facebook#2402
typeof window.__REACT_VERSION__ === 'undefined',
'Multiple instances of React have been initialized on the same page. ' +
'Currently initializing React v' + REACT_VERSION + ' but another instance of React v' +
window.__REACT_VERSION__ + ' was already initialized'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think versions matter that much. The real issue is, they won't work together well, IMO it's worth explicitly stating that. Also a link to an official fb.me gist would be nice but that's up to maintainers.

@ivan-kleshnin
Copy link

👍 I was beaten by that not once

@four43
Copy link

four43 commented Apr 8, 2015

+1, Thanks for this. Indicating there is a problem is the first step, then we can work on solving the problem. This seems like a quick indication that would save a lot of hassle of not knowing what the problem.

@jimfb
Copy link
Contributor

jimfb commented Apr 9, 2015

It has been long enough, I think everyone has had a chance to see this. Looks good to me, merging.

jimfb added a commit that referenced this pull request Apr 9, 2015
…of-react-on-same-page

Warn when multiple instances of React are loaded on the same page
@jimfb jimfb merged commit f1cd867 into facebook:master Apr 9, 2015
@JedWatson
Copy link
Contributor

👍 brilliant, thanks for this.

@gaearon
Copy link
Collaborator

gaearon commented Apr 9, 2015

👍

@robertknight
Copy link
Contributor Author

An issue with this at the moment is that it won't produce a warning if mixing React < 0.14 with React >= 0.14. Are there any other global properties on window or document that this warning could check for to detect earlier versions of React?

@robertknight robertknight deleted the 2402-warn-multiple-copies-of-react-on-same-page branch April 9, 2015 11:51
@sophiebits
Copy link
Collaborator

I don't think this PR is the best solution here. It's legitimate to load two copies of React if you keep them separate – for example, you might want to embed a third-party script which bundles its own copy of React and only renders into its own subtree. As long as you create and render elements with the same copy of React, you won't run into problems (as far as I know of).

I'd prefer something more like #3332 where we warn in problematic cases and leave people alone if they're not mixing things in a problematic way. This PR also doesn't catch cases where you use multiple copies of React on the server in a bad way (since it guards its global assignment in a canUseDOM), which is another reason I don't like tacking on a global.

In any event, this adds a lot of log spew to the tests and that needs to be fixed.

@robertknight
Copy link
Contributor Author

@spicyj - Apologies about the log spam in the tests, I hadn't noticed as the tests that produce the warnings run early on and I'd only seen the all-green output at the end. I agree that tacking on a global is not particularly pretty, but the solution in #3332 wouldn't have caught the issue I ran into. See #2402 (comment) .

Looking at the various posts that @gaearon linked to in the description of 2402 the problem in many of those cases was when using Browserify or Webpack to create an app bundle and inadvertently ending up with dependencies and the main app code pulling in different versions of React. The second time this happened to me the result was actually that only parts of React were duplicated because of Browserify's deduplication of modules that didn't change between the two versions.

@gaearon - Do you know if there is a way to prevent Webpack from including multiple versions of a dependency in the same bundle, or a way that React could detect if this happens?

@robertknight
Copy link
Contributor Author

Here is a possible alternative which would only handle the case where multiple copies of React are bundled into the same Browserify/Webpack bundle or set of bundles that are part of the same app.

This relies on the 'process' object being available in both Browserify/Webpack and being shared across all modules in the app. This could also (optionally - by testing process.browser) be used server-side in Node. It should also fix the spam in the tests.

// warnIfModuleIsDuplicated.js

// check for duplicated modules in the same Node app or Webpack/Browserify bundle.
// This relies on the 'process' module being shared by all modules in the same bundle
// but not between bundles from different vendors.
//
// - This could check process.browser to only warn in Webpack/Browserify bundles.

function warnIfModuleIsDuplicated(name, version) {
    if (typeof process === 'undefined' || typeof process.env !== 'object') {
        return;
    }

    var versionKey = name + '_VERSION';
    var existingVersion = process.env[versionKey];
    if (existingVersion) {
        console.warn('Multiple copies of ' + name + ' were loaded. Found existing version ' + existingVersion +
          ' while loading version ' + version);
    }
    process.env[versionKey] = version;
}

module.exports = warnIfModuleIsDuplicated;

// in React.js
require('warnIfModuleIsDuplicated')('React', module.exports.version);

Does this seem like a better approach? If so I'll submit a PR along these lines.

@zpao
Copy link
Member

zpao commented Apr 10, 2015

@spicyj should we revert this while we sort it out?

@syranide
Copy link
Contributor

@zpao Off-topic, but fixing the tests would be awesome, many currently use multiple (partial?) React instances simultaneously and just happens to work at current. It is a blocker for a PR I had to keep the concatenated IDs stored internally, leaving only minimal sequential IDs in the DOM, say what you want about that PR but it is something we'll want to fix eventually at least :)

@sophiebits
Copy link
Collaborator

@zpao Yeah, let's for now. #3646.

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.

Warn when two versions of React are used alongside
10 participants