fix: handle Storybook `webpackFinal` merging separately

[jtoar]

Closes https://github.com/redwoodjs/redwood/issues/4980. This PR destructures out the webpackFinal keys from the Redwood and user Storybook config before webpack-merge does its thing. We merge it ourselves, then add it back later.

I tested this with the simplest case possible, so this definitely deserves real-world testing. My redwood app had the following storybook.config.js file:

// web/config/storybook.config.js

module.exports = {
  webpackFinal: async (sbConfig, { configType }) => {
    console.log('Fix me')
    return sbConfig
  },
}

Currently, this breaks Storybook completely, but with this PR, it seems to work again.

@virtuoushub the discussion you linked to (https://github.com/storybookjs/storybook/discussions/18150) got me thinking: maybe all the Storybook stuff we're doing in @redwoodjs/testing should be a Storybook preset? That is, we add the storybook.config.js file to the create-redwood-app template, with the following:

// web/config/storybook.config.js

module.exports = {
  addons: ['@redwoodjs/storybook']
}

And we rename storybook.config.js to storybook.main.js of course. Or have a new Storybook dir in web/config that mirrors the way the Storybook docs detail it.

[netlify[bot]]

✅ Deploy Preview for redwoodjs-docs ready!

Name	Link
🔨 Latest commit	89fd6a525fee8e19a7397467d9f217a7d46e81d9
🔍 Latest deploy log	https://app.netlify.com/sites/redwoodjs-docs/deploys/62d1528df9d0aa00090ad032
😎 Deploy Preview	https://deploy-preview-5957--redwoodjs-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[virtuoushub]

🔥 - thanks @jtoar ! I used some examples from https://storybook.js.org/docs/react/builders/webpack#extending-storybooks-webpack-config to smoke test this and it worked great so far!

one thing to note is we will probably want to do the same thing for managerWebpack:

...
  const {
    webpackFinal: baseWebpackFinal,
    managerWebpack: baseManagerWebpack,
    ...baseConfigRest
  } = baseConfig;

  const {
    webpackFinal: userStorybookConfigWebpackFinal,
    managerWebpack: userStorybookConfigManagerWebpack,
    ...userStorybookConfigRest
  } = userStorybookConfig;
...
  mergedConfig.managerWebpack = async (config, options) => {
    let configFinal = await baseManagerWebpack(config, options);

    if (userStorybookConfigManagerWebpack) {
      configFinal = await userStorybookConfigManagerWebpack(
        configFinal,
        options
      );
    }

    return configFinal;
  };

[dac09]

@jtoar just to note the cli/lib/merge seems to handle this already - and does it quite elegantly - up to you if you want to use that version or this one!

[virtuoushub]

Gentle bump - is this still relevant given #4900 ? Or would we use that PR's util here instead of what we have?

[virtuoushub]

Gentle bump - is this still relevant given #4900 ? Or would we use that PR's util here instead of what we have?

another gentle bump (@jtoar, @dac09).

I seem to be somewhat blocked over at https://github.com/redwoodjs/example-storybook/pull/109 due to this not being in the framework

[dac09]

@virtuoushub - didn't the changes in #4900 make it possible to configure it now?

[virtuoushub]

@virtuoushub - didn't the changes in #4900 make it possible to configure it now?

@dac09, I am not observing this.

In my testing it looks to be that whatever a framework user puts in web/config/storybook.config.js for webpackFinal (e.g. override file), at least in some part overrides what is in the framework config over at packages/testing/config/storybook/main.js, and does not actually merge the two.

This makes sense to me if I look at #4900, I do not see the actually new merge utils being used in packages/testing/config/storybook/main.js, It appears as if they were mostly used for the initial setup command over atpackages/cli/src/lib/configureStorybook.js.

So I think:

@jtoar just to note the cli/lib/merge seems to handle this already - and does it quite elegantly - up to you if you want to use that version or this one!

Is close to correct, in that #4900 has a different mechanism (i.e. implementation) to handle what this PR ( #5957 ) is doing if we want to leverage that instead. However, I think we still need to wire it up in packages/testing/config/storybook/main.js for us to get the benefits of those cli/lib/merge changes.

[replay-io[bot]]

10 replays were recorded for 643c6b98a91b61a982cc57df4fe0658a91aebf19.

0 Failed

10 Passed

• requireAuth graphql checks
• useAuth hook, auth redirects checks
• RBAC: Admin user should be able to delete contacts
• RBAC: Should not be able to delete contact as non-admin user
• Smoke test with dev server
• Smoke test with rw serve
• Loads Cell mocks when Cell is nested in another story
• Loads Cell Stories
• Loads MDX Stories
• Mocks current user, and updates UI while dev server is running

View test run on Replay ↗︎