Use babel-loader from react-scripts

[mrmckeb]

Issue: #5183

What I did

This is an attempt to resolve babel-loader from react-scripts, rather than installing our own.

How to test

This is not ready to merge, it needs local testing and unit tests will be added.

[tmeasday]

This seems great, but probably @igor-dv or @ndelangen should approve it as they are closer to this code

[codecov]

Codecov Report

Merging #5308 into next will increase coverage by 0.21%.
The diff coverage is 11.11%.

@@            Coverage Diff             @@
##             next    #5308      +/-   ##
==========================================
+ Coverage   40.85%   41.06%   +0.21%     
==========================================
  Files         612      612              
  Lines        8486     8449      -37     
  Branches      535      411     -124     
==========================================
+ Hits         3467     3470       +3     
+ Misses       4927     4924       -3     
+ Partials       92       55      -37

Impacted Files	Coverage Δ
lib/core/src/server/build-static.js	0% <ø> (ø)	⬆️
.../core/src/server/utils/load-custom-babel-config.js	0% <0%> (ø)	⬆️
app/react/src/server/framework-preset-cra.js	0% <0%> (ø)	⬆️
lib/core/src/server/manager/manager-config.js	0% <0%> (ø)	⬆️
app/react/src/server/cra-config.js	87.5% <100%> (ø)	⬆️
lib/components/src/ScrollArea/ScrollArea.tsx
lib/components/src/tooltip/WithTooltip.stories.tsx
lib/components/src/ActionBar/ActionBar.tsx
lib/components/src/tooltip/ListItem.stories.tsx
lib/components/src/form/form.stories.tsx
... and 95 more

---

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 bdc537e...074d0f3. Read the comment docs.

[mrmckeb]

@igor-dv, this should be ready for a re-review now. Thanks!

[VincentLanglet]

@mrmckeb Hello ! Did you try this changes ?

I did, in order to do a patch locally. This is what I did in the dist folder :

function isBabelLoader8 {
  return true

And

    resolveLoader: {
      modules: ['node_modules', _path.default.join(getReactScriptsPath(), 'node_modules')],
    },

It load but wait forever with these messages :

info => Loading create-react-app config.
webpack built b41fbb2dacde6d54f87d in 2415ms 

I found the issue come from here, @storybook/core/dist/server/dev-server.js, the managerPromise.

Promise.all([managerPromise, previewPromise])
    .then(([managerStats, previewStats]) => {
      router.get('/', (request, response) => {
        response.set('Content-Type', 'text/html');
        response.sendFile(path.join(`${outputDir}/index.html`));
      });
      router.get(/\/sb_dll\/(.+\.js)$/, (request, response) => {
        response.set('Content-Type', 'text/javascript');
        response.sendFile(path.join(`${dllPath}/${request.params[0]}`));
      });
      router.get(/\/sb_dll\/(.+\.LICENCE)$/, (request, response) => {
        response.set('Content-Type', 'text/html');
        response.sendFile(path.join(`${dllPath}/${request.params[0]}`));
      });
      router.get(/(.+\.js)$/, (request, response) => {
        response.set('Content-Type', 'text/javascript');
        response.sendFile(path.join(`${outputDir}/${request.params[0]}`));
      });

      webpackResolve({ previewStats, managerStats, managerTotalTime, previewTotalTime });
    })
    .catch(e => webpackReject(e));

The Promise.all([managerPromise, previewPromise]) is never resolved but Promise.all([previewPromise]) is.

Looking down to the managerPromise:

const managerPromise = loadManagerConfig({
    configType,
    outputDir,
    configDir,
    cache,
    corePresets: [require.resolve('./manager/manager-preset.js')],
  }).then(
    config =>
      new Promise((resolve, reject) => {
        webpack(config).watch(
          {
            aggregateTimeout: 1,
            ignored: /node_modules/,
          },
          (err, stats) => {
            managerTotalTime = process.hrtime(startTime);
            if (err) {
              reject(err);
            } else if (stats.hasErrors()) {
              reject(stats);
            } else {
              resolve(stats);
            }
          }
        );
      })
  );

Here, stats.hasErrors() return true. First question: Why the reject is silent ?

If I console.log the stats, I can see

 missingDependencies:
      SortableSet [Set] {
        '/Users/node_modules',
        '/Users/vlanglet/Project/myPath/node_modules/babel-loader',
        '/Users/vlanglet/Project/myPath/node_modules/babel-loader.js',
        '/Users/vlanglet/Project/myPath/node_modules/babel-loader.json',
        '/Users/vlanglet/Project/myPath/node_modules',
        '/Users/vlanglet/Project/node_modules',
        '/Users/vlanglet/node_modules',
        '/node_modules',
        _sortFn: undefined,
        _lastActiveSortFn: undefined,
        _cache: undefined,
        _cacheOrderIndependent: undefined
       } 

So either there is something else to do in your PR, or I got it wrong.
I'll keep looking for a solution.

[VincentLanglet]

@mrmckeb Adding

resolveLoader: {
  modules: ['node_modules', _path.default.join(process.cwd() + '/node_modules/react-scripts', 'node_modules')],
}

In the @storybook/core/dist/server/manager/manager-webpack-config.js fix the problem.

So I think you have to do something in @storybook/core/src/server/manager/manager-webpack-config.js.

[mrmckeb]

Hi @VincentLanglet, I've been a little stalled on this work sorry, but plan to pick it back up this week.

This was more of a POC, it's not ready to merge yet.

[VincentLanglet]

@mrmckeb Hi ! I know it was a POC, I just tried to help you to finish this PR.
Thanks to your starting work I was able to create a patch (with patch-package) in my project ; I explained to you what I did more.

[VincentLanglet]

Hello @mrmckeb ; do you need some help ? :)

[mrmckeb]

Hi @shilman, sorry I was away, and I'm back home again. I've just completed a PR for create-react-app (CRA) to add templates support, and we were thinking of ways to support this at that time - but didn't come up with a silver bullet.

There were a few suggestions, and I think I'll start this branch again (as it was originally built on Storybook v4) and update the PR this week (or this weekend).

@igor-dv had previously stated that he felt this was a little "hacky" *#5308 (diff) - and whilst I don't agree, I can certainly try to find another approach. The issue is that it seems that core takes a lot of the responsibility, and I'm not sure that there will be a cleaner approach to solve this.

[mrmckeb]

Hi all, this version is a little simplified and tests are now working. Sorry again for the long delay before reworking.

CC @shilman @igor-dv @VincentLanglet

If you're still seeing the previous issue, @VincentLanglet, it would be really helpful to see the output of an npm ls on your system.

[VincentLanglet]

@mrmckeb Since the new storybook version, I have no issue and I don't need any patch.
But it's maybe only temporary.

My npm ls is too big for a copy paste...

[mrmckeb]

Thanks @VincentLanglet - I meant to write npm ls babel-loader 😆 Sorry!

BUT, I have reproduced your issue and I'm working on a fix.

This issue will reappear next time CRA and Storybook go out of sync, so it needs to be fixed at some stage. Thanks :)

[vercel]

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://monorepo-git-feature-cra-babel-loader.storybook.now.sh

[mrmckeb]

Thanks @igor-dv, I've solved the last issue. This is now (finally) ready for a re-review.

[shilman]

let's let the webpack gurus weigh in but LGTM

[igor-dv]

In what case there will be an error here ? do we want to hide it ?

[mrmckeb]

Good question @igor-dv. This happens when babel-loader isn't in the root of node_modules. When testing, this was sufficient to handle the error and still load successfully.

[igor-dv]

Something is happening with CI. It shows that the artefact size decreased dramatically. Maybe the build-storybook is not working or something ?

[mrmckeb]

@igor-dv, I've rebased and I'm pushing up again to see if that helps.

Otherwise, do you think it could be related to this? It's the only related change I can think of - but seems odd.
https://github.com/storybooks/storybook/pull/5308/files#diff-329a7cb6ceb533e8db66ba0c81dda4a0R123

[igor-dv]

Can't thing about something specific.. can you try to build-storybook locally and check if it's working ?

[mrmckeb]

@igor-dv It looks like it was that change. I've removed for now as it isn't needed for this PR I believe (correct me if I'm wrong).

[igor-dv]

@mrmckeb, but then the managerWebpack won't work in the framework presets, am I wrong ?

[igor-dv]

I mean, that what fixed it, right ?

[mrmckeb]

@igor-dv I only made that change to align with dev-server - a change someone else had already made before I got there... but it looks like that may be required for build-static too.

Passing in all options was breaking everything, so I'll try just passing in what's required. Waiting for the CI...

[mrmckeb]

@igor-dv That fixed it, the previously failing tests are now passing. I've just squashed my commits and will merge once the CI has completed again.

Thanks for your help.