Fix error with new CRA Webpack config

[mrmckeb]

Issue: #5077

What I did

@igor-dv, it looks like the code you had worked fine - except for the require.resolve on the first file, which caused the whole script to fall over if the file did not exist. This fix has been tested locally and works.

How to test

In node_modules/react-scripts, remove the current webpack configs and use the new config from create-react-app

EDIT: As [email protected] has been released, this can be tested by installing create-react-app, and running, then downgrading to [email protected], and running again.

[codecov]

Codecov Report

Merging #5074 into next will not change coverage.
The diff coverage is 0%.

@@           Coverage Diff           @@
##             next    #5074   +/-   ##
=======================================
  Coverage   35.05%   35.05%           
=======================================
  Files         593      593           
  Lines        7348     7348           
  Branches     1000     1000           
=======================================
  Hits         2576     2576           
  Misses       4259     4259           
  Partials      513      513

Impacted Files	Coverage Δ
app/react/src/server/cra-config.js	47.05% <0%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81a44e0...48bdc8e. Read the comment docs.

[ndelangen]

This seems to break our example test, can you confirm this fix?

[mrmckeb]

My apologies @ndelangen, I've fixed that - I didn't test against the old config filename properly.

I've confirmed the new fix locally with the old .dev/.prod suffixes, and the new filename.

[mrmckeb]

The CLI tests are now passing for this work, the failures are related to something else:

./src/stories/MyButton.vue

@igor-dv, let me know if you have any suggestions here.

[shilman]

@mrmckeb Aside from the vue failure, there's also a failure directly related to this change in cli-test-latest-cra:

info => Loading create-react-app config.
ERR! Error: Cannot find module '/tmp/storybook/node_modules/react-scripts/config/webpack.config.dev'
ERR!     at Function.Module._resolveFilename (module.js:548:15)
ERR!     at Function.resolve (internal/module.js:18:19)
...

As I understand it, your update is intended to solve this problem .. but then I don't understand why that test is failing. Confused... 😕

[appbak3r]

#5077 to many PRs created already

[mrmckeb]

@shilman From what I can see, this test actually installs Storybook from npm.

I can see that the new react-scripts is being installed:

├─ [email protected]

Then, I can see that @storybook/react is not using the build from this PR:

 start-storybook -p 9009 -s public --smoke-test --quiet
info @storybook/react v4.1.3
info 
info => Loading static files from: /tmp/storybook/lib/cli/test/run/react-scripts-latest-fixture/public .
info => Loading presets
info => Loading presets
info => Loading custom addons config.
info => Loading create-react-app config.
ERR! Error: Cannot find module '/tmp/storybook/node_modules/react-scripts/config/webpack.config.dev'

This is the issue that the PR aims to resolve. I've tested locally, but this CI test seems flawed in that it pulls in the published version.

As this is now affecting users (as of two days ago), it would be great to get this merged and released ASAP.

As a sanity check, you can install create-react-app locally, then simply rename the webpack.config file to webpack.config.dev.

npx create-react-app my-app

I should have tested this more thoroughly with @igor-dv before [email protected] was released, and I'm sorry I didn't.

[Hypnosphi]

From what I can see, this test actually installs Storybook from npm.

No, it should use a local linked version

[igor-dv]

Cool, thanks

[simonbuchan]

Note that this change breaks storybook with react-scripts < 2.1.2, when run from a yarn workspace or other case where react-scripts has been hoisted to a higher node_modules.

In this case, getReactScriptsPath() falls back to relative path "react-scripts" which causes the fs.existsSync() check to fail.

Easy workaround is upgrade react-scripts, but really this whole file should just be using require('react-scripts/package.json'), require('react-scripts/config/webpack.config.js'), etc., since that's more resilient to packager nonsense going forward (e.g. PNP)

[mrmckeb]

@simonbuchan, unfortunately we can't just require react-scripts as people can fork react-scripts and name it whatever they want. However, it would be great to solve the issue you've raised - do you have any other suggestions?

[simonbuchan]

Oh man, that's a tough one, with that as a restriction, you're doing pretty good!

For this PR removing the require.resolve() for pathToWebpackConfig breaks the fallback case where pathToReactScripts is just "react-scripts". Putting that back with a catch for the resolution error replacing the fs.existsSync() check seems like it would work?

My preference would be to offer some option to override the scripts module used at that point, (e.g. ts-loader taking the alternative typescript package to use) - but that's a breaking change for those users.

Note that support for following the binstub is already broken on windows, as binstubs are not symlinks on windows, so realpath() still gives you the .bin path. Not sure how to fix that, other that trying to pluck the path out of the stub script (gross!)

[mrmckeb]

Interesting, I use WSL on Windows, so I've not run into that issue @simonbuchan, but great to know as we'll likely hit it at some point.

The reason we removed require.resolve was that it caused the subsequent check to always fail, which was a critical issue when [email protected] hit.

However, I'm a little confused as to why this PR broke functionality for you - as I would expect require.resolve and require to operate the same way in terms of resolution? Can you explain a little more? Also, what version of react-scripts are you using? Thanks again.

[simonbuchan]

It's a problem when getReactScriptsPath() returns just "react-scripts", which I assumed was because I was running in a yarn workspace where react-scripts had been hoisted to a higher node_modules directory, but could also be due to being on windows.

In that case, the later fs.existsSync() for the pre 2.1.2 version of react-scripts dev or prod config file fails, and it falls back to blindly requiring the post 2.1.2 config file. So easy fix is just tell pre 2.1.2 people using windows or workspaces or whatever to bump react-scripts and call it a day, but you could probably do something nicer?