Run tests against source, CJS, and UMD builds

[pshrmn]

Take advantage of Jest's moduleNameMapper to test the cjs, es, and umd builds in addition to the source code. The React repo has a similar process for testing its builds.

The react-router and react-router-dom tests skip the minimized umd builds because those have tests for warnings that get stripped from the minimized bundle.

[pshrmn]

Hopefully this is never necessary, but it does provide a verification that there aren't issues with the builds.

This does require the builds to be built before testing. A pretest could be added to ensure they run on the latest build, but this might have issues with double builds during CI. There could also be issues with Jest's watch mode running against stale builds. Maybe this should leave the test script to run jest on the source and add a test:builds command to test against the builds?

[timdorr]

I was going to suggest that for the scripts. My concern is losing the ability to pass args to Jest easily. I tend to push back against scripts as aliases (npm run test:watching:coverage:regression:foo:bar and every combination of those options...), but this case makes sense.

[pshrmn]

As long as the default runs against source, we can always use npx jest in packages and pass args that way. That could be an argument for testing builds by default, but it doesn't really matter to me which way this goes.

[pshrmn]

I was curious how React handles the tests on code that is stripped out in production builds (logs) and it turns out they use custom Jest matchers that just pass when the test isn't run in __DEV__ mode.

[mjackson]

I really like the idea of running our tests on our builds, so thank you for this PR, @pshrmn. I'm sorry but I've been doing a lot of refactoring and adding new tests this past week, so this PR needs to be updated. But I think it's definitely worth it if we can make it work.

[mjackson]

Just FYI: I'm done messing around with the build for version 4.4 so it should be safe to rework this now. @timdorr actually found a bug in the CJS build this morning that I didn't catch because I changed the tests to pull directly from the modules directories so we don't have to create a build just to run the tests. But testing the builds would've been a nice way to automate this.

[timdorr]

Crap, I was working on this too... I'll let @pshrmn take over, but had some commits for moving the tools/test.js file into scripts/test.js and a __DO_NOT_USE__RouterContext export from react-router, so we can get the same instance around. We do the same thing in Redux to get our uniquely generated action to the tests.

[timdorr]

@pshrmn Did you back out the changes to react-router-config? That's where I was stuck too.

[pshrmn]

@timdorr yeah, the "context mismatch" caused by importing from different builds (e.g. CJS tests import <Router> from source, but renderRoutes() imported <Switch> from the CJS build) was annoying and I wanted to get the other changes made.

I also dropped testing the ES builds. I think those were actually working in my old commits, but I rebased everything together before rebasing on master and don't remember exactly how that was done 🤦‍♂️ (Babel config?).

[pshrmn]

And feel free to add anything else to this commit.

[mjackson]

This looks great, thanks @pshrmn! :)

A __DO_NOT_USE__ export on react router sounds like the right way to go so we can keep those tests how they were originally written.

[mjackson]

the "context mismatch" caused by importing from different builds (e.g. CJS tests import from source, but renderRoutes() imported from the CJS build) was annoying

Can you please say more about this? I'm trying to think it through but I'm missing something.

[pshrmn]

react-router-config's renderRoutes() function imports the Switch from react-router. For a CJS build, that Switch import is coming from the react-router package's CJS build (packages/react-router/Switch.js).

The renderRoutes() tests import a Router from react-router, but they import it from the source code (packages/react-router/modules/Router.js).

The context created in the source code and the context created in the CJS build aren't compatible (different createContext() calls), so the <Switch> cannot read from the <Router>'s context.

[pshrmn]

This could probably be solved by also mapping the react-router package in jest.config.js, but I didn't know if that would clash with the new Babel configuration and I didn't want to break that.

[mjackson]

Thanks for the clarification, @pshrmn. That makes sense. And the reason we don't have the same problem with the CJS react-router-dom build is because it does not treat react-router as an external, like the CJS react-router-config build does. Is that right?

But we would have the exact same problem with react-router-dom if its tests imported something from react-router, right? Because those would be pulling from source code as well.

[pshrmn]

Yes, the react-router-dom tests all import from react-router-dom, so there aren't any issues.

And yes, importing the MemoryRouter from react-router in Link-test.js throws an error.

import { MemoryRouter } from "react-router";
import { HashRouter, Link } from "react-router-dom";

Invariant Violation: You should not use <Link> outside a <Router>

[pshrmn]

I was just looking at how to implement this for react-router-config and I noticed that the latest react-router-config UMD build is bundling its own version of the Route and Switch (which caused it to balloon from 2.81kb to 69.5kb). This is related to the transform-imports Babel plugin (I believe that because it rewrites the imports, the Rollup config globals no longer match).

Maybe updating to Babel 7 to take advantage of JS Babel configs would be helpful here?

[mjackson]

I tried updating to Babel 7 but decided to just stick with 6 for now. If you have time to look into upgrading, I'd be all for it. The JS config files are nice. But we were already doing that before with the tools/babel-preset.js file. The reason I got rid of it is because I was trying to simplify.

[mjackson]

Sorry this is a little off topic, but FWIW I think we should stop using the transform-imports plugin. For starters, it introduces a lot of indirection into the import process. And secondly, I'd like to drop support for require('react-router/Router') anyway and ship just 2 bundles: one for dev and another for prod (same as React does). So transforming import { MemoryRouter } from 'react-router' into import MemoryRouter from 'react-router/MemoryRouter' is something we won't have to do anymore. We can still support it in version 4 with a CommonJS stub for each file, but we can change the way we require things in our code before that so that we won't rely on it anymore. I think that would help simplify things in situations like this.

[mjackson]

@pshrmn You were correct, the react-router-config UMD bundle was pulling in code from the ESM build of react-router. I fixed it in 5fb11b3.

I also noticed another weird issue, which is that the website was actually pulling from the CJS build in one spot and the ESM build in another. I fixed it in 6460fe0, but it caught me off guard. I've always kind of used import { Router } from 'react-router' and import Router from 'react-router/Router' interchangeably, but they're not anymore due to the changes in the context object. This is probably something a lot of people are going to have problems with.

[mjackson]

Hey @pshrmn, was just wondering if you'd still like to get this merged. Since you opened this PR, we've separated out the Jest config files (so we can use moduleNameMapper) and converted all our import statements in tests to pull from the bundle name instead of directly from the source files, so it should be fairly straightforward to implement at this point. Right now all the tests are just running the CommonJS build.

BTW, thank you for getting the ball rolling here! I'm going to feel so much better once we get this merged.

[pshrmn]

Yeah, I can take a look at this soon to see what all of these conflicts are.

[pshrmn]

Not a whole lot to this now that the Jest configs were already moved.

[pshrmn]

I added the react-router-config tests back in now that the context issues are resolved.

[mjackson]

Awesome, thanks @pshrmn! I may keep the test script but may also get rid of it and just use the build matrix in Travis to run all the different builds. Then, if we want to run tests against a specific build we can just set the TEST_ENV variable.

[mjackson]

Huh, looks like the source tests are failing to import the react-is library for some reason in Route.js. Any idea why? I think Jest should just be using whatever we have in node_modules, and since we aren't hoisting anymore, react-is should be there... but I can't get TEST_ENV=source jest to run locally inside packages/react-router w/out seeing the error.

EDIT: You can see the error in the most recent build

[pshrmn]

Oh shoot, I noticed that but forgot to mention it. Sorry!

It looks like react-is uses named exports, but the NODE_ENV switch converts them to a default export. If you change the source to import * as ReactIs from "react-is"; or import { isValidElementType } from "react-is", then the source tests works, but the build fails. Huzzah, prop types!

[pshrmn]

@TrySound mind if I pick your brain about this sutation?

Given a module that uses named exports

// node_modules/module/real-module.js
exports.thing = true;

but its "main" module has a default export.

// node_modules/module/index.js
module.exports = require("./real-module.js");

I believe that when Rollup converts the module to import/export syntax, it will only have a default export, so Rollup expects something like this:

// src/index
import Module from "module";
console.log(Module.thing);

but if the code is run without Rollup (e.g. testing the source code), it expects the import to look like this:

// src/index.js
import { thing } from "module";
console.log(thing);

How would you support both use cases? Would you use rollup-plugin-commonjs's namedExports? Or maybe I'm just missing a fundamental aspect and there is an easier solution?

[mjackson]

It looks like react-is uses named exports, but the NODE_ENV switch converts them to a default export. If you change the source to import * as ReactIs from "react-is"; or import { isValidElementType } from "react-is", then the source tests works, but the build fails.

Ah, well in that case I think this means that you found an actual bug! Huh, imagine that... 😅

[mjackson]

@pshrmn Could this be fixed by testing the ESM build directly (i.e. esm/react-router.js) instead of the source code? Seems like something we should probably be doing since that's what other people who are using ES modules will be using, right?

EDIT: Ah, nm. I forgot we can't use the esm file because Jest won't know how to compile the modules. Could we pass in a custom babel config that just handles modules in that case?

[pshrmn]

I got it working locally by updating the Rollup config to:

const commonjsOptions = {
  include: /node_modules/,
  namedExports: {
    "node_modules/react-is/index.js": ["isValidElementType"]
  }
};

and the Route.js import to:

import { isValidElementType } from "react-is";

I like having source code tests because you can run npx jest (especially with the --watch and --verbose flags) without having to re-build the package.

[mjackson]

Perfect, thanks for investigating @pshrmn. I found the same thing and pushed it in 90ecb35 😅

[TrySound]

Yep, I would use namedExports option.