Support async configs for scripts (fix #190)

[spautz]

This is intended to address issue #190, and it uses the same approach as was written there.

I added a second option for loading the craco config: loadCracoConfigAsync, which is exactly like loadCracoConfig except it returns a promise. loadCracoConfig is unchanged, so it still works the same for the APIs that use it.

With that addition, the built-in scripts (build, start, test) can support craco.config.js files which return a promise or an async function, while API calls like createJestConfig work exactly like before (i.e. the caller has to resolve the config as an object.)

This seemed similar to the "config might be a function" discussion in PR #195, so I tried to match that by adding validation that checks for promises.

[patricklafrance]

Hi @spautz

Thanks for the PR.

I do have a few questions!

1- I haven't played with scripting for a while, i've been stuck in React components for the last year.... How does node scripts handle promises at the root? If the async code takes too long to return, won't the script terminate before the CRA code is executed?

If you do have relevant documentation to refresh my mind, that would help a lot!

2- The config object is also loaded by a babel-jest transform script. Does babel-jest transform support async? Otherwise it would mean that CRACO offer partial support async and I am not sure I do like that.

3- Since start / build / test script now expect to await on a promise, does it still support a CRACO config returning an object literal?

Thank you,

Patrick

[spautz]

1- I haven't played with scripting for a while, i've been stuck in React components for the last year.... How does node scripts handle promises at the root? If the async code takes too long to return, won't the script terminate before the CRA code is executed?

It keeps the script alive until the promise finishes. This seems to apply to any promises or timeouts: it keeps the process alive even if there's an unreachable anonymous promise somewhere.

Under the hood, I think it waits for the event queue to be empty: https://nodejs.org/api/process.html#process_event_exit says that exit will happen after "The Node.js event loop no longer having any additional work to perform".

2- The config object is also loaded by a babel-jest transform script. Does babel-jest transform support async? Otherwise it would mean that CRACO offer partial support async and I am not sure I do like that.

I don't think it does: I played around with a few of the APIs under lib/, but trying to make them wait for the promise spiraled into a very large changeset -- and even if babel-jest supports promises, changing the craco functions to async would be a breaking change for anybody using them.

I viewed this as being similar to the partial support for functions (where the caller has to resolve the config themselves before passing it to the craco function they want), but after looking at the code a little more I think (1) this is already not supported for the babel-jest transform script (i.e., a caller cannot pass in the config themselves when it requires the babel-jest transform), and (2) it may be possible to add support for both.

Current behavior:

• Both the test script and the createJestConfig function call mergeJestConfig (and the craco config -- already resolved -- has to be passed in)
• If the config overrides both babel and jest.babel, and is adding presets or plugins, then configureBabel() will use jest-babel-transform.js
• jest-babel-transform.js loads cracoConfig itself, from the config file -- so somebody who passed their config to createJestConfig may get an invalid result.

Proposal:

• Have jest-babel-transform.js export a function that takes a config and creates the transform from it. Now it will work with async configs and with createJestConfig when using those overrides.
• For backwards compatibility, the default export from jest-babel-transform.js can still load cracoConfig itself, from the config file: it'll still work as-is when used directly.

Does that seem reasonable?

3- Since start / build / test script now expect to await on a promise, does it still support a CRACO config returning an object literal?

It does, yes: the async/await automatically wraps the object literal in a promise. The start / build / test scripts should support object literals, functions, promises, or functions that return promises (so long as it eventually resolves to an object literal).

[spautz]

I implemented the "function that takes a config and returns the transform" proposal in commit 52980fb (with a minor bugfix in f2080ca). It should now fully support async everywhere, and should also apply transforms if somebody passes a config directly to the API instead of using a file.

That commit migrates the main body of jest-babel-transform.js to a function (create-jest-babel-transform.js), which mergeJestConfig uses. The jest-babel-transform.js file returns the config-file-based transform as before. I tried to follow existing conventions for modules and exports. I'm happy to make changes if this approach doesn't seem right.

I've tested this against my own project and a few test configs, but I'm not sure how rigorously to exercise the transform overrides. Are there any thorough test examples for the "override both babel and jest.babel to add plugins/presets" case?

[patricklafrance]

Thanks, i'll have a look.

Do you know of any popular projects who support async config file?

To be honest, this does seem like a weird use case to me.

[patricklafrance]

I implemented the "function that takes a config and returns the transform" proposal in commit 52980fb (with a minor bugfix in f2080ca). It should now fully support async everywhere, and should also apply transforms if somebody passes a config directly to the API instead of using a file.

That commit migrates the main body of jest-babel-transform.js to a function (create-jest-babel-transform.js), which mergeJestConfig uses. The jest-babel-transform.js file returns the config-file-based transform as before. I tried to follow existing conventions for modules and exports. I'm happy to make changes if this approach doesn't seem right.

I've tested this against my own project and a few test configs, but I'm not sure how rigorously to exercise the transform overrides. Are there any thorough test examples for the "override both babel and jest.babel to add plugins/presets" case?

Sorry, I haven't added any test coverage to the project. I usually create a sandbox locally and try a few use cases.

[spautz]

Do you know of any popular projects who support async config file?

Webpack is the one for my current situation: they've supported promises in config files since Webpack 2. https://webpack.js.org/configuration/configuration-types/#exporting-a-promise

Jest supports promises (they have an async example for jest.config.js near the top of https://jestjs.io/docs/en/configuration), and Rollup as well (a few paragraphs into https://rollupjs.org/guide/en/#configuration-files). There may be others, but Webpack is really the big one for me.

[patricklafrance]

I tryed to run yarn jest on your fork with a boilerplate CRA and I get the following error:

    Jest encountered an unexpected token

    This usually means that you are trying to import a file which Jest cannot parse, e.g. it's not plain JavaScript.

    By default, if Jest sees a Babel config, it will use that to transform your files, ignoring "node_modules".

    Here's what you can do:
     • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
     • If you need a custom transformation specify a "transform" option in your config.
     • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.

    You'll find more details and examples of these config options in the docs:
    https://jestjs.io/docs/en/configuration.html

    Details:

    SyntaxError: C:\Dev\oss\craco-sandbox\sandbox\src\App.test.js: Unexpected token (6:31)

      4 | 
      5 | test('renders learn react link', () => {
    > 6 |   const { getByText } = render(<App />);
        |                                ^
      7 |   const linkElement = getByText(/learn react/i);
      8 |   expect(linkElement).toBeInTheDocument();
      9 | });

      at Parser._raise (node_modules/@babel/parser/src/parser/location.js:241:45)
      at Parser.raiseWithData (node_modules/@babel/parser/src/parser/location.js:236:17)
      at Parser.raise (node_modules/@babel/parser/src/parser/location.js:220:17)
      at Parser.unexpected (node_modules/@babel/parser/src/parser/util.js:149:16)
      at Parser.parseExprAtom (node_modules/@babel/parser/src/parser/expression.js:1142:20)
      at Parser.parseExprSubscripts (node_modules/@babel/parser/src/parser/expression.js:539:23)
      at Parser.parseMaybeUnary (node_modules/@babel/parser/src/parser/expression.js:519:21)
      at Parser.parseExprOps (node_modules/@babel/parser/src/parser/expression.js:311:23)
      at Parser.parseMaybeConditional (node_modules/@babel/parser/src/parser/expression.js:263:23)
      at Parser.parseMaybeAssign (node_modules/@babel/parser/src/parser/expression.js:211:21)

Test Suites: 1 failed, 1 total
Tests:       0 total
Snapshots:   0 total
Time:        1.688s
Ran all test suites.
error Command failed with exit code 1.

craco.config.js is returning an empty object literal:

module.exports = {
};

When I switchto CRACO master branch, everything works fine.

[spautz]

Can you give any more information about your environment and what steps you're running? I was able to run each of the CRACO scripts in a a new boilerplate CRA without issue, using both Node 12 and Node 14.

Here are the exact steps I did: what were yours?

1. npx create-react-app my-app from scratch
2. yarn add @craco/craco in my-app
3. Add the near-empty craco.config.js, and update the build, test, and start scripts to use craco <command> instead of react-scripts <command>
4. Test everything against the current release
5. yarn link from the packages/craco/ directory in my fork
6. yarn link @craco/craco in my-app (and verified that my-app's node_modules/@craco/craco/ had the right files)
7. Test everything against the fork

I also tried it against the fork only (without first installing the current release of CRACO), using npm for everything instead of yarn (just to be certain), and by pointing the dependency in my-app at the actual path to the package on disk (instead of linking through yarn/npm).

[patricklafrance]

Hi @spautz

Ended up working not sure what happened yesterday.

I did the same steps as you did with Node 14.4.

Have you tried the test script with babel plugins?

For the following craco config:

module.exports = {
    eslint: {
        enable: false
    },
    babel: {
        plugins: ["babel-plugin-jsx-control-statements", "babel-plugin-react-require"]
    }
};

I get the following error message when running Jest:

$ craco test --verbose
craco:  Project root path resolved to:  C:\Dev\oss\craco-sandbox\sandbox
craco:  Config file path resolved to:  C:\Dev\oss\craco-sandbox\sandbox\craco.config.js
craco:  Override started with arguments:  [
  'C:\\Program Files\\nodejs\\node.exe',
  'C:\\Dev\\forks\\craco\\packages\\craco\\scripts\\test.js',
  '--verbose'
]
craco:  For environment:  test
craco:  Found craco config file at:  C:\Dev\oss\craco-sandbox\sandbox\craco.config.js
craco:  Applied craco config plugins.
craco:  Found jest config at:  C:\Dev\oss\craco-sandbox\sandbox\node_modules\react-scripts\scripts\utils\createJestConfig.js
craco:  Overrided require cache for module: C:\Dev\oss\craco-sandbox\sandbox\node_modules\react-scripts\scripts\utils\createJestConfig.js
craco:  Overrided Jest config provider.
craco:  Overrided Jest config.
craco:  Testing CRA at:  C:\Dev\oss\craco-sandbox\sandbox\node_modules\react-scripts\scripts\test.js
craco:  Overrided Jest Babel transformer.
craco:  Applied Jest config plugins.
craco:  Merged Jest config.
TypeError: customTransformer.includes is not a function
    at C:\Dev\oss\craco-sandbox\sandbox\node_modules\jest-config\build\normalize.js:300:40
    at Array.forEach (<anonymous>)
    at setupBabelJest (C:\Dev\oss\craco-sandbox\sandbox\node_modules\jest-config\build\normalize.js:285:40)
    at normalize (C:\Dev\oss\craco-sandbox\sandbox\node_modules\jest-config\build\normalize.js:605:3)
    at readConfig (C:\Dev\oss\craco-sandbox\sandbox\node_modules\jest-config\build\index.js:163:46)
    at readConfigs (C:\Dev\oss\craco-sandbox\sandbox\node_modules\jest-config\build\index.js:373:26)
    at C:\Dev\oss\craco-sandbox\sandbox\node_modules\@jest\core\build\cli\index.js:155:58
    at Generator.next (<anonymous>)
    at asyncGeneratorStep (C:\Dev\oss\craco-sandbox\sandbox\node_modules\@jest\core\build\cli\index.js:108:24)
    at _next (C:\Dev\oss\craco-sandbox\sandbox\node_modules\@jest\core\build\cli\index.js:128:9)
error Command failed with exit code 1.

[spautz]

After some research, I've learned I completely misunderstood the transform config: it can only ever be a mapping from a file glob to a module name or path -- never a function, only a string it can require(). In hindsight, the Jest docs are clear about that. Several people have wanted to use a function instead of a file path over the years, but that's not really supported (jest #1468, jest #7015)

I've found a solution that seems to work, though, based on this comment -- but I'm worried it might be too hacky. What do you think?

The basic idea:
We can make cracoConfig (the runtime config, not necessarily loaded from a file) available inside jest-babel-transform.js by passing it through the overall config for Jest. Then we can make a plain transformer which reads the cracoConfig and then creates + redirects to a new transformer the first time it's run.

This is not too far off from how ts-jest's transformer does things: it uses Jest's config as a global space as well, and redirects to the process() of the babelJestTransformer found there. Using globals and creating a transform on-the-fly still feels dirty to me, though, but I haven't found anything else that actually works properly. Commit bc0831c shows this change.

[patricklafrance]

Thanks for the investigating @spautz

The proposed solution looks fine to me. As you said it's a bit hacky but this project IS A BIG HACK so it's fine haha.

I'll run a few tests later tonight.

[patricklafrance]

What do you need this for? It is called without a craco config. Is it why you also load the craco config inside the transformer?

[spautz]

This populates any other keys that babelJest.createTransformer generates: in addition to process(), they make a getCacheKey function and set a canInstrument prop. That's the only thing it's used for.

We could set all keys directly here -- it just seems possibly more future-proof to start with whatever babel-jest creates and override just the process(). I can update it if you'd prefer. Or, for about the same work we could probably override all functions (i.e. have getCacheKey do the same "generate and use jestBabelTransform from the config" that process does), although in my tests it seemed unnecessary.

[patricklafrance]

Did a few tests and it does work great!

One last thing... could you also update the installation docs to showcase that a config file can return an async function?

Thanks!

[spautz]

I've updated the installation docs and made all the changes you commented on, except for the one left open (the question about calling createJestBabelTransform() without a config). I can change that further if you'd like.

I tested the latest change against the same configs I'd used before.

[patricklafrance]

Merged, thank you :)

Will release tonight!

[patricklafrance]

Released in https://www.npmjs.com/package/@craco/craco/v/5.7.0