Skip to content

wip: Add updated Storybook example for [email protected] #27

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

wip: Add updated Storybook example for [email protected] #27

wants to merge 1 commit into from

Conversation

seniorquico
Copy link

The Webpack configuration files for Storybook appear to have been split up across a few different packages. It's not quite as straightforward as the first time I looked at the source of the configs for these examples.

This PR is my first stab at changes to get the Storybook example working with the latest [email protected] to address #26. I'd like to try to dig through the new Webpack configuration files in the new Storybook packages and the latest [email protected] to see if there are other important preset/plugin differences.

Copy link
Owner

@strothj strothj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current version of react-scripts (2.0.0), uses a custom loader instead of the normal babel-loader to work around an issue they are having: https://github.com/facebook/create-react-app/blob/master/packages/react-scripts/config/webpack.config.dev.js#L233.

This will need to be updated for now to take that into account:

Essentially, both of these patterns need to be checked:
https://github.com/strothj/react-app-rewire-typescript-babel-preset/blob/master/packages/rewire/rewireWebpack.ts#L114

@strothj
Copy link
Owner

strothj commented Sep 27, 2018

Totally agree with removing the monorepo example. Thanks for the work.

@strothj strothj dismissed their stale review October 6, 2018 06:25

Whoops, looks like I was mistaken there.

@strothj
Copy link
Owner

strothj commented Oct 6, 2018

Hello,

Would you mind rebasing this?

What do you think about reusing the Babel loader from react-scripts:

const babelLoader = getReactScriptsBabelLoader(env);
config.module.rules.unshift(babelLoader);
function getReactScriptsWebpackConfig(env) {
  const reactAppRewired = require("react-app-rewired");
  const rewiredConfigOverrides = require("react-app-rewired/config-overrides");
  const craWebpackConfig = require(path.join(
    reactAppRewired.paths.scriptVersion,
    `config/webpack.config.${env === "PRODUCTION" ? "prod" : "dev"}`
  ));
  const rewiredWebpackConfig = rewiredConfigOverrides.webpack(
    craWebpackConfig,
    env === "PRODUCTION" ? "production" : "development"
  );
  return rewiredWebpackConfig;
}

function getReactScriptsBabelLoader(env) {
  const { getLoader } = require("react-app-rewired");
  const reactScriptsWebpackConfig = getReactScriptsWebpackConfig(env);
  const babelLoader = getLoader(reactScriptsWebpackConfig.module.rules, rule =>
    Boolean(
      rule.test && rule.test.toString() === /\.(ts|tsx|js|jsx)$/.toString()
      // Later versions of react-scripts add support for JavaScript Modules:
      // rule.test.toString() === /\.(ts|tsx|js|mjs|jsx)$/.toString(),
    )
  );
  return babelLoader;
}

@seniorquico
Copy link
Author

I'm not versed enough in the various packages/changes to have a strong opinion on reusing the loader in react-scripts. I did a quick check, and Storybook v4 doesn't appear to use the loader from react-scripts.

https://github.com/storybooks/storybook/blob/2ef180d4886bee84b3203445fc3cdc0bcde47812/lib/core/src/server/config/webpack.config.dev.js#L70-L80
https://github.com/storybooks/storybook/blob/master/app/react/src/server/framework-preset-react.js

Do you have some insight to elaborate on the pros/cons of using the loader from react-scripts vs sticking closer to Storybook's configurations? e.g., if this was switched to the loader from react-scripts, would users be likely to encounter build issues after updating to Storybook v4 and/or a version of CRA with built-in support for TypeScript (in reference to their PR 4837)?

@strothj
Copy link
Owner

strothj commented Oct 10, 2018

I was working on migrating a project to the new build in PR 4837 and came up with this in the process:

This is using the latest Storybook alpha that uses Babel 7.

The findMonoRepo is the old code from here:
https://github.com/facebook/create-react-app/blob/22353ecf02db21f7bff6a0c088ef7f0631f29fd6/packages/react-dev-utils/workspaceUtils.js
That's only important if you want monorepo support. Just posting it here in case someone else finds it useful. I haven't had a chance to test this extensively yet.

import { findMonorepo } from "@secret-project/lib-build-tools";
import assert from "assert";
import path from "path";
import { Configuration, RuleSetLoader, RuleSetRule } from "webpack";

export default function(
  _baseConfig: Configuration,
  _env: "DEVELOPMENT" | "PRODUCTION",
  defaultConfig: Configuration,
): Configuration {
  const scriptRule = getScriptRule(defaultConfig);

  // Allow transpiling of source files from source directory of monorepo
  // packages directly. With this we can import uncompiled source directly
  // from monorepo packages.
  addMonorepoSourcePathsToScriptRule(scriptRule);

  const babelOptions = getBabelLoaderOptions(scriptRule);

  // Add TypeScript support.
  scriptRule.test = /\.[jt]sx?$/;
  babelOptions.presets.push(require.resolve("@babel/preset-typescript"));
  defaultConfig.resolve!.extensions!.unshift(".tsx", ".ts");

  // Support for Create React App v2 SVG Component imports.
  babelOptions.plugins.push([
    require.resolve("babel-plugin-named-asset-import"),
    {
      loaderMap: {
        svg: {
          ReactComponent: "@svgr/webpack?-prettier,-svgo![path]",
        },
      },
    },
  ]);

  return defaultConfig;
}

function getScriptRule(config: Configuration) {
  const scriptRule = config.module!.rules.find(
    rule => rule.test!.toString() === String(/\.jsx?$/),
  );
  if (!scriptRule) throw new Error("Could not locate script loader.");
  return scriptRule;
}

function addMonorepoSourcePathsToScriptRule(scriptRule: RuleSetRule) {
  if (!scriptRule.include || !Array.isArray(scriptRule.include)) {
    throw new Error("Expected include to be string array.");
  }

  const monorepoPkgPaths = findMonorepo(path.resolve(__dirname, "..")).pkgs;
  const monorepoSourcePaths = monorepoPkgPaths.map(p => path.join(p, "src"));
  scriptRule.include.push(...monorepoSourcePaths);
  scriptRule.exclude = [/[/\\\\]node_modules[/\\\\]/];
}

function getBabelLoaderOptions(scriptRule: RuleSetRule) {
  let babelLoader: RuleSetLoader;
  try {
    babelLoader = (scriptRule.use as RuleSetLoader[]).find(
      loader => loader.loader === "babel-loader",
    )!;
    assert(babelLoader);
  } catch (e) {
    throw new Error("Could not locate babel loader.");
  }

  return babelLoader.options as Record<string, any[]>;
}

@strothj
Copy link
Owner

strothj commented Oct 10, 2018

Thank you for the help. I'm going to close this because Create React App will be incorporating the TypeScript features.

@strothj strothj closed this Oct 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants