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

Add a warning about using different builds #6361

Closed
mjackson opened this issue Sep 30, 2018 · 12 comments
Closed

Add a warning about using different builds #6361

mjackson opened this issue Sep 30, 2018 · 12 comments

Comments

@mjackson
Copy link
Member

mjackson commented Sep 30, 2018

Before 4.4, you could do this w/out problems:

import MemoryRouter from 'react-router-dom/MemoryRouter';
import { Link } from 'react-router-dom';

<MemoryRouter>
  <Link /> // Error!
</MemoryRouter>

It's not very obvious, but those two import statements may actually reference different builds. The first one is using our CommonJS build, and the second one may be using the ES modules or the CommonJS build, depending on how your build is setup and where you told it to find files from the react-router-dom package. If you're using webpack (which you probably are) then it's probably pulling from the ES modules build.

In the case where <Link> is coming from a different build (or any other component, e.g. <Route>), you will currently run into an error like this:

You should not render a <Link> outside a <Router>

This is because of the context object mismatch, which is a problem introduced by our using the new context API. Basically, each build has its own context object, and components from different builds can't be used together.

For this reason, I think it'd be nice to provide a better warning for folks who might be pulling the files from different builds accidentally, like I was doing (see 6460fe0). Maybe something like:

You are using two different builds of React Router in the same application. To fix this, please make sure you always import from the same build. [link to a gist or some docs page that explains the issue more thoroughly].

@mjackson
Copy link
Member Author

Also, thank you to @pshrmn who took the time to explain this issue to me in #6209 (comment)

@timdorr
Copy link
Member

timdorr commented Oct 1, 2018

We had originally added the transform-imports function in #5095. The motivation was bundle sizes from duplicate modules, but this has an overlapping concern that's now exposed as a header.

Regardless of the approach taken, we'll want to make sure we keep the bundle size in mind. Fixing one will likely fix the other. It'll be annoying, but I'd suggest setting up a test repo, npm linking in RR and RRD, and checking output sizes as you try various things.

Rollup has got good DCE built-in nowadays and Webpack respects the sideEffects: false key we added to the package.json's. So, I think it should be easy enough to solve by just following best practices.

@mjackson mjackson added this to the 4.4 milestone Oct 10, 2018
@jimthedev
Copy link
Contributor

jimthedev commented Oct 24, 2018

Is this also a concern for libraries that import {Link} from "react-router-dom"; and then are imported into an application which has its own router?

More specifically if I have a library of more complex components that contain various <Link> and <NavLinks inside them, how can an application import components from my library and use the correct context (the application router context) even though the code lives in a separate bundle?

@pshrmn
Copy link
Contributor

pshrmn commented Oct 24, 2018

@jimthedev as long as every react-router-dom (technically react-router because that is where the context is created) import refers to the same module, there isn't an issue.

Issues are most likely to be cause by:

  1. Mixing with "cherry picked" imports where one uses the ESM build and the other the CJS build.
import { Route } from "react-router-dom"; // esm
import Link from "react-router-dom/Link"; //cjs
  1. react-router version mismatch, which should only happen if the package version gets hard-coded.

@mjackson
Copy link
Member Author

mjackson commented Nov 6, 2018

Now that we have a single entry point for each build, this could probably be as easy as defining a global variable like globals.ReactRouterModuleFormat = 'umd'. Then, at the top of each build we do something like

if (globals.ReactRouterModuleFormat && globals.ReactRouterModuleFormat !== "cjs") {
  // warn
}

And we would only do this in __DEV__ ofc. Whatcha think?

@hugomallet
Copy link

Hi,

I have a package A (built with CRA) that uses another package B built using babel in a monorepo configuration.

The code looks like this

// Package A
import { BrowserRouter as Router } from 'react-router-dom';
import B from '@monorepo/B';

export default () => (
    <Router>
        <B />
    </Router>
);
// Package B
import { Link } from 'react-router-dom';

export default () => (
    <div>
        <Link />
    </div>
);

They both have the dependency react-router-dom: ^5.0.0

But i get the following error :
Error: Invariant failed: You should not use <Link> outside a <Router>

@pshrmn
Copy link
Contributor

pshrmn commented Mar 28, 2019

@hugomallet can you verify that they are using the same build? It is possible that one is importing the CJS build while the other is importing the ESM build. I think that the most likely scenario for that would be if one of the packages doesn't use the module field in its package.json while the other does, but I'm a bit fuzzy on how bundlers pick which build to use.

@prayerslayer
Copy link

Hey @hugomallet we had literally this exact same problem here since a couple of hours. Managed to fix it by enforcing the ESM build by importing every react-router component from react-router-dom/esm/react-router-dom. I don't know yet what in our toolchain apparently chose different builds.

@hugomallet
Copy link

@pshrmn How can i check that they use the same build ?
Package B is only passed to babel, not bundled. Only package A is bundled (using CRA).

I have downgraded to 4.3.1 for now.

@pshrmn
Copy link
Contributor

pshrmn commented Mar 29, 2019

I would find the package in your node_modules folder, add console.log() statements to each of them, and run the application.

@remix-run remix-run deleted a comment from MuhammadSulman Apr 24, 2019
@draftitchris
Copy link

@hugomallet @pshrmn we are having this exact same problem using a lerna monorepo with yarn workspaces. Except in our case we are importing A into B taking from hugomallets example. Have triple checked all versions are the same and even attempted enforcing an import from react-router-dom/esm/react-router-dom but we still go the same invariant error. Downgrading to 4.3.1 works as well for us for now

@hugomallet
Copy link

I think the workaround with lerna is to hoist dependencies. Webpack seems to bundle dependencies based on relatives paths (not packages versions). So if two files require react from two different locations then react will be duplicated in the bundle (which is not permitted with react-router@5).

@lock lock bot locked as resolved and limited conversation to collaborators Jul 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants