facebook / create-react-app

Set up a modern web app by running one command.
https://create-react-app.dev
MIT License
102.7k stars 26.84k forks source link

require.context doesn't work in tests #517

Closed suchipi closed 8 years ago

suchipi commented 8 years ago

I sometimes use webpack's require.context to automatically load files from the filesystem. Example:

Directory structure:

- components
  - Button.js
  - Icon.js
  - index.js
- docs.js

index.js:

// `require` in every file except index.js from the current directory, then
// add their default export to module.exports based on their filename.
// eg module.exports["Button"] = require("./Button.js");
const req = require.context(".", false, /^\.\/(?!index).*\.js$/);
req.keys().forEach((fileName) => {
  const exportName = fileName.replace("./", "").replace(".js", "");
  module.exports[exportName] = req(fileName).default;
});

docs.js:

import * as components from "./components";
Object.entries(components).forEach(([componentName, ComponentClass]) => {
  /* generate docs for `componentName` using `ComponentClass` */
});

This example is somewhat contrived because index.js could just be replaced with:

import Button from "./Button";
import Icon from "./Icon";

export { Button, Icon }

but when you're importing hundreds of files, this is both helpful and powerful; it's easy to write a require.context that treats named exports differently, for example:

const req = require.context(".", false, /^\.\/(?!index).*\.js$/);
req.keys().forEach((fileName) => {
  const exportName = fileName.replace("./", "").replace(".js", "");
  // next line changed, now we export both the default export and the named docExamples export
  module.exports[exportName] = [req(fileName).default, req(fileName).docExamples];
});

This pattern allows you to co-locate documentation examples with your code, which discourages documentation getting out of sync with the component.

Although this example only addresses documentation, this pattern can be useful anywhere you have a lot of files with correlated features and enforcing an export signature convention is feasible and discourages code rot.


This is all possible using create-react-app today. However, it all breaks when you try to run your tests (in 0.3.0-alpha), because jest (naturally) doesn't use webpack:

Using Jest CLI v14.1.0, jasmine2
 FAIL  src/__tests__/some-test.js (0s)
 Runtime Error
  - TypeError: require.context is not a function

Is require.context "too obscure" of a webpack feature? Should create-react-app start disallowing it or warning about its usage? Where and how do we draw the line on which webpack features should be supported and which should not be? Or, should we try to support require.context in the test runner enrivonment? If so, where and how do we draw the line on which webpack features should be supported in the test environment?

gaearon commented 8 years ago

Is require.context "too obscure" of a webpack feature? Should create-react-app start disallowing it or warning about its usage? Where and how do we draw the line on which webpack features should be supported and which should not be?

Yep, it is too obscure in my opinion. I’m happy to take a pull request that adds a lint rule against its usage.

If we don’t document something in howto, we don’t support it.

gaearon commented 8 years ago

Where and how do we draw the line on which webpack features should be supported and which should not be?

I’d say any custom extensions to require are not going to be officially supported, and may break at any time. We don’t actively plan to break them but we may. We do, however, understand that many people need code splitting, so we will keep require.ensure support until we switch to Webpack 2 which offers it via System.import.

vjeux commented 8 years ago

cc @cpojer for evaluating implementing require.context in jest

cpojer commented 8 years ago

I really don't want to add this to Jest. require.context seems like a really bad API and hard to statically analyze. We really shouldn't encourage use of this because something is "hard to type" or long.

This should be solvable by a custom __mocks__ file for the index module that exports all of the modules OR creates a Proxy object for the index module that has getters during runtime.

gaearon commented 8 years ago

Yea, I agree with @cpojer here. It isn’t friendly to ES6 modules either and will break any tooling that assumes them.

suchipi commented 8 years ago

All comments makes sense. Thanks for the discussion. Future readers: I was able to workaround this by providing mocks for each file that uses require.context, as @cpojer indicated. But as it is unsupported, I do not recommend using it. I plan on changing my code to not use it.

suchipi commented 8 years ago

Just had a thought: what about stuff like:

require("./something/" + name);

Webpack generates a context module when you pass a dynamic path like this (which is the same kind of module that can be manually created with require.context). Should this behavior also be warned against? Normal node can do dynamic requires, but imports must be static.

I'm not sure what the behavior in jest is for this case, but I expect that it works, though mocking it might be inconvenient.

cpojer commented 8 years ago

Inside of tests this is fine, you can require dynamic modules as much as you'd like and sometimes that is useful.

Outside of tests it is bad because those cases cannot be statically analyzed.

draperd commented 7 years ago

Although I understand the rationale of the decision on the discussion, I thought it might be valuable to provide another use case of require.context as additional feedback:

In my case I'm using create-react-app to develop components that will be imported and consumed by other projects within our organizations. Part of the requirement is localization and both with my own implementation and using react-intl I've needed to use require.context in order pass an argument to a utility function or HoC enhancer to provide a path the localization bundles for that component.

Without require.context Webpack will attempt to load the bundles in a location relative to the utility function and not the component.

So with require.context I can do this...

i18nUtils.addBundle({
    requireContext: require.context("./i18n", false),
    bundleName: "BaseField"
})

...and have the utility function merge the message like this...

bundleMessages = input.requireContext(`./${input.bundleName}.${language}.json`);
merge(this.messages, bundleMessages);

Based on other discussions I do understand why create-react-app doesn't and should tie itself to proprietary module loading capabilities... however, I just want to highlight another use case of require.context.

One improvement that could be made would be for the linting to output a warning or even an error when require.context is used in the project source... because require.context works fine in the application, but then fails in the tests.

So this is of course a symptom of a broader requirement for simple i18n support, but I came across this thread when searching for information on the error message I was seeing.

gaearon commented 7 years ago

Doesn't this mean you're bundling all locales all the time?

draperd commented 7 years ago

Well, unfortunately yes...

But that's a symptom of doing this entirely on the client-side. Our previous Spring MVC based solution handled localization bundles on the server so that the client only received bundles for the requested locale.

However, in the context of what we're trying to do (which is to provide re-usable components for 3rd parties to use in custom applications accessing REST APIs of our platform) it's a something that we need to live with.

The issue is somewhat mitigated by trying to ensure that we only load messages of the components that are used. A simpler but more expensive solution would be to just provide a single bundle containing all of the messages for all of the components (especially given that some of the components don't even have a localization requirement).

I completely agree that loading multiple bundles is not ideal, but without having control of the server this is the best solution. Again, this problem is unlikely to be common I would assume that most people just want to build their own application.

gaearon commented 7 years ago

Is there a problem with explicitly importing them in the code? After all require.context is just sugar for doing that.

draperd commented 7 years ago

How would I pass it as a reference?

Let's say for example I have src/components/fields/BaseField that has localization bundles in the folder src/components/fields/i18n.

If I pass either ./i18n/ as a path to my util function at src/utilities/i18n/i18nUtils.js then it will attempt to load the bundles from src/utilities/i18n/i18nUtils/i18n (and not from the folder relative to BaseField).

Now I get a bit hazy about whether or not there is support for some root folder symbol, and I also don't know what would happen to other projects consuming my published project with regards to explicit links.

The nice thing about require.context is that I know that Webpack will make sure the path is relative to BaseField and that all possible locale variants will be made available and the appropriate ones will be used at runtime (at least I think that's what's going to happen, it seems to work in practice! ;-) )

Maybe I'm missing something really obvious because I've been staring at this problem for so long?

gaearon commented 7 years ago

I don't mean passing a path. I mean writing imports by hand:

var context = {
  en: require('./i18n/en'),
  fr: require('./i18n/en'),
  // ...
}

This doesn't rely on Webpack magic.

draperd commented 7 years ago

Well, only that I'd have to write a shed load more code which I'd like to avoid if possible. By going through a utility it means that I don't necessarily have to update the component code when support for new locales are added (our localization team would work separately to the component team and provide localization bundles outside of development sprints so the translated bundles could arrive at any time).

Abstracting away behind a utility means that I can change the utility without having to change all my components. We got to over 300 components in our last application development framework - I'm trying to keep maintenance overheads down with this approach.

gaearon commented 7 years ago

Do you have a specific proposal on how we could support it better without encouraging people to use non-standard features?

draperd commented 7 years ago

No, unfortunately not... and just to be clear, I'm not expecting you to magic a solution out of thin air, I've just found from experience that it's useful to receive feedback. I'm not expecting require.context to be suddenly supported, I just wanted to share my experience.

I'll continue to try to find a good solution to the problem and will provide an update if and when I do.

I do appreciate you taking the time to answer though and provide suggestions.

draperd commented 7 years ago

One thought that does cross my mind that I might investigate is updating my local scripts to perform the Webpack build without minification (that outputs to some directory other than build) and then have the tests import from the generated resources.

There are a few obvious issues with this... I don't think the build would declare explicit exports for each component so it would be necessary to import the full build layers into the test pages (which is probably not a good thing). Also, for continuous testing during development it would be necessary to trigger a build on each source file saved ... this would also rely on the output having a static name (rather than including a checksum) in order for Jest to be able to track changes to it.

It probably wouldn't be particularly fast... but essentially it would avoid the issue of testing the raw, unpacked source files. This would mean that it wouldn't matter what build system was used because you'd always be testing the final output so it wouldn't make any difference what proprietary build features the code was using...

Any thoughts on that? ... it obviously feels quite "hacky", but at the end of the day it doesn't actually change what the final build output would be.

UPDATE: Initial experimenting with this suggests that it will be too slow to be useful during development and the Webpack generated output can't be imported into the tests anyway... back to the drawing board :(

draperd commented 7 years ago

The final solution I've gone with is just to have an i18n folder and place all bundles in there, this means that I can specify paths that will always be relative to my i18nUtils module. The bundle location mirrors the component location. The limitation of this approach is that consumers of our project won't be able to use the utility in their own application... however, I'll probably provide support for passing require.context to the utility as an option (albeit one that we ourselves won't use within the project so that we can make use of the test framework provided)

daedalus28 commented 6 years ago

FYI for anyone who encounters this issue - instead of relying on require.context or manually building a file like @gaearon suggested, you can use directory-metagen automatically generate import/requires for files in a directory. It can output es6 modules, AMD, commonjs, and more. It even has a CLI :)

ndelangen commented 6 years ago

Here's how we've solved this issue: https://github.com/storybooks/storybook/blob/babel-7/scripts/jest.init.js#L10

https://github.com/smrq/babel-plugin-require-context-hook

hackhat commented 6 years ago

I wonder why jest doesn't just use an environment given. I think is not a good idea to jest running in a way, and the real app in a different way.

Would be nice to run your tests through webpack exactly the same as your app instead of jest having his own special stuff. So many inconsistencies and things to keep in sync between your webpack config and jests config. For example if you include polifills you need to make sure both webpack and jest config has the same ones.

In my case I have a folder of migrations and I need to import them all. I don't think would be a good practice to include each one manually as you might forget to include it when you add a new one, but anyway, this is a nice to have.

hackhat commented 6 years ago

By the way I "solve" it by not calling require.context directly from my code. I just added an utility like this:

const fs = require('fs');
const path = require('path');

export const requireContext = (base = '.', scanSubDirectories = false, regularExpression = /\.js$/) => {
  // @ts-ignore
  if (typeof require.context !== 'undefined') {
    // @ts-ignore
    return require.context(base, scanSubDirectories, regularExpression);
  }

  const files = {};

  function readDirectory(directory) {
    fs.readdirSync(directory).forEach((file) => {
      const fullPath = path.resolve(directory, file);

      if (fs.statSync(fullPath).isDirectory()) {
        if (scanSubDirectories) readDirectory(fullPath);

        return;
      }

      if (!regularExpression.test(fullPath)) return;

      files[fullPath] = true;
    });
  }

  readDirectory(path.resolve(__dirname, base));

  function Module(file) {
    return require(file);
  }

  // @ts-ignore
  Module.keys = () => Object.keys(files);

  return Module;
};
timkindberg commented 5 years ago

@hackhat You cannot use require.context with variables. Every param has to be a hard-coded literal for static analysis. How is that working for you?