Build: Overhaul dev script & compile-babel & compile-tsc #17338

ndelangen · 2022-01-26T13:21:15Z

What I did

See comment on code explaining details, but in short:

remove dev script
As far as I am aware, it's slow for everyone, and therefore not used.
change the watch mode of yarn build to use the prepare script
previously, the prepare script was circumvented when watching.
I made it so the prepare script takes a --watch argument instead.
remove the babel --copy-file flag where possible
I was able to remove this everywhere except lib/cli
It's a bad pattern because it requires stupid cleanup to not over publish.
It's also slower then it needs to be.
Placing non-source files in source, and having babel copy them over is just a bad pattern IMHO.
The copy-files flag ignores the --ignore flag, hence the need for the cleanup step.
remove noise from prepare
No longer do we get a ton of noise from the command-line from TSC or babel. It's just 1 simple message or an error.
improve error reporting from prepare
Errors compiling now have a better error reporting from TSC
cleanup
Certain things were just not used at all.

How to test

Well obviously everything should just work as before

change the watch mode of yarn build to use the prepare script add a --watch flag to prepare script remove the babel --copy-file flag where possible remove noise from prepare improve error reporting from prepare cleanup

nx-cloud · 2022-01-26T13:21:17Z

☁️ Nx Cloud Report

CI ran the following commands for commit 9c77ed4. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 1 target

nx run-many --target=prepare --all --parallel --max-parallel=15 -- --optimized

Sent with 💌 from NxCloud.

ndelangen · 2022-01-26T13:22:48Z

.babelrc.js

@@ -50,6 +50,7 @@ module.exports = {
    ],
    ['@babel/plugin-proposal-class-properties', { loose: true }],
    ['@babel/plugin-proposal-private-methods', { loose: true }],
+    ['@babel/plugin-proposal-private-property-in-object', { loose: true }],


SO this is an interesting one:

I was tweaking around with the output from the compile-babel command, and when it piped stdout to the main process, it started complaining about loose not being configured correctly.

It suggested I added this, and when I did the error disappeared. 🎈

This was also an issue I faced when using the babel config for the prebundler work.

ndelangen · 2022-01-26T13:23:49Z

.circleci/config.yml

@@ -221,7 +221,6 @@ jobs:
          name: Run E2E tests
          # Do not test CRA here because it's done in PnP part
          # TODO: Remove `web_components_typescript` as soon as Lit 2 stable is released
-          # TODO: Add `angular` as soon as Storybook is compatible with Angular 13


cleanup, AFAICS we DO support angular13 now, it's literally in the list below

ndelangen · 2022-01-26T13:27:35Z

addons/docs/package.json

@@ -40,6 +40,7 @@
    "common/**/*",
    "ember/**/*",
    "html/**/*",
+    "svelte/**/*",


So the svelte app, has a XYZ.svelte decorator, which we do not parse/compile/transpile at all. To us, it's NOT source.

before this file was copied over (multiple times) to all the dist folders (duplicated).
besides being inefficient, and increasing the size of our packages (very minuscule), we can just point to a file relative to the package root, because why have babel copy the file 3 times if it can just exist in 1 place instead?

ndelangen · 2022-01-26T13:28:36Z

addons/docs/src/frameworks/svelte/prepareForInline.ts

-// @ts-ignore
-import HOC from './HOC.svelte';


See point above

So the svelte app, has a XYZ.svelte decorator, which we do not parse/compile/transpile at all. To us, it's NOT source.

before this file was copied over (multiple times) to all the dist folders (duplicated).
besides being inefficient, and increasing the size of our packages (very minuscule), we can just point to a file relative to the package root, because why have babel copy the file 3 times if it can just exist in 1 place instead?

ndelangen · 2022-01-26T13:30:43Z

addons/storysource/src/register.tsx

@@ -2,7 +2,7 @@ import React from 'react';
 import { addons } from '@storybook/addons';

 import { StoryPanel } from './StoryPanel';
-import { ADDON_ID, PANEL_ID } from '.';


This just makes it easier to actually find references to this file, the import X from '.' could resolve into many different things in some environments.

In fact, during my prebundling work this was actually a problem, so let's avoid it shall we?

ndelangen · 2022-01-26T13:31:20Z

app/svelte/src/client/preview/index.ts

+const {
+  configure: coreConfigure,
+  clientApi,
+  forceReRender,
+} = start(renderToDOM, {
  decorateStory,
  render,
 });

-export const {
-  setAddon,
-  addDecorator,
-  addParameters,
-  clearDecorators,
-  getStorybook,
-  raw,
-} = clientApi;
+export const { setAddon, addDecorator, addParameters, clearDecorators, getStorybook, raw } =
+  clientApi;


just formatting

ndelangen · 2022-01-26T13:32:09Z

docs/contribute/code.md

@@ -143,7 +143,7 @@ npx sb@next link --local /path/to/local-repro-directory

 <details>

-<summary>`yarn dev` watches everything but is resource-intensive</summary>


I removed the dev command entirely, let's all use yarn build <packagenames> [--watch] instead.

ndelangen · 2022-01-26T13:32:35Z

lib/builder-webpack4/package.json

-    "dll/**/*",
-    "types/**/*",


cleanup, neither of these folders exist anymore

ndelangen · 2022-01-26T13:33:54Z

lib/builder-webpack4/package.json

@@ -31,8 +31,7 @@
  },
  "files": [
    "dist/**/*",
-    "dll/**/*",
-    "types/**/*",
+    "templates/**/*",


Add the templates directory, I've moved the non-source files we previously had babel copy them into dist (multiple times). Now they are located in the package root in a templates directory. (and so they have to be in the allow-list for npm!

ndelangen · 2022-01-26T13:39:56Z

lib/builder-webpack4/src/preview/iframe-webpack.config.ts

-      await readTemplate(path.join(__dirname, 'virtualModuleModernEntry.js.handlebars')),
+      await readTemplate(
+        require.resolve(
+          '@storybook/builder-webpack4/templates/virtualModuleModernEntry.js.handlebars'


So here's a discussion point:

I can resolve to a file in the current package's root in (at least) 2 ways:

How I've done it now: refer to the whole npm package.

I could also opt to use pkg-dir with the __dirname of the current file.

Option 2 works by working up with the fs module from the specified directory until it found a package.json file, then return that directory.

So option 2 would look like this instead:

path.resolve( path.join( pkgDir.sync(__dirname), './templates/virtualModuleModernEntry.js.handlebars' ) )

In fact I HAD to use the above technique in 1 place because of unit-tests mocking the file-system.

SO should I use option 2 everywhere?

For some reason it feels safer to me, it feels sort of weird to require('x/...') when we are already in package x. I'm not sure what the guarantees around that are.

Another alternative would be do it relative require('../../../templates/...). I guess that would presuppose our dist folder structure and be kind of annoying.

ndelangen · 2022-01-26T13:40:09Z

lib/builder-webpack5/package.json

-    "dll/**/*",
-    "types/**/*",


ndelangen · 2022-01-26T13:41:51Z

lib/channel-websocket/tsconfig.json

+  "include": [
+    "src/**/*"
+  ],


When working on the stdout and stderr coming from compile-tsc, this came up.

I ran into a problem where TSC tried to compile files in dist. It refused to do it, because files already existed.
This will ensure tsc will only compiled things in src.

ndelangen · 2022-01-26T13:42:06Z

lib/cli/package.json

@@ -38,8 +38,7 @@
    "dist/**/*",
    "README.md",
    "*.js",
-    "*.d.ts",
-    "versions.json"


cleanup: This file does not exist

ndelangen · 2022-01-26T13:46:15Z

lib/cli/scripts/generate-sb-packages-versions.js

+  await writeFile(
+    path.join(__dirname, '..', 'src', 'versions.ts'),
+    dedent`
+      // auto generated file, do not edit
+      export default ${JSON.stringify(packageToVersionMap, null, 2)}
+    `
+  );
 };


This script now generates a versions,ts file, instead of a version.json file.

It's easy enough to do (as you can see).

The reason I did this, is because *.json files are not considered "source files", and so when removing --copy-files flag from babel, this file went missing.

By generating the .ts file instead this problem goes away, and we actually get auto-complete.
I could make it even more type-safe, but that seemed not worth the time.

@shilman I can revert this if you'd like...

We should remove the script if we're not using it...

ndelangen · 2022-01-26T13:49:15Z

lib/cli/src/versions.ts

@@ -0,0 +1,58 @@
+// auto generated file, do not edit


@shilman I did not see ANY references of the script that generates this (but the file is up to date, so I assume something is using it?)

Could you let me know if another script is generating this?

ndelangen · 2022-01-26T13:49:49Z

lib/components/src/blocks/Story.tsx

@@ -1,7 +1,7 @@
 import global from 'global';
 import React, { createElement, ElementType, FunctionComponent, Fragment } from 'react';

-import type { Parameters } from '@storybook/api';


@storybook/api is not a dependency of this package, but csf is...

ndelangen · 2022-01-26T13:50:27Z

lib/core-common/package.json

@@ -31,8 +31,8 @@
  },
  "files": [
    "dist/**/*",
-    "dll/**/*",


ndelangen · 2022-01-26T13:51:13Z

lib/core-common/package.json

@@ -31,8 +31,8 @@
  },
  "files": [
    "dist/**/*",
-    "dll/**/*",
    "types/**/*",


This actually DOES exist here, I assume the package.json was just copied over and over when creating new packages, without looking if it made sense.

ndelangen · 2022-01-26T13:52:24Z

lib/core-common/src/utils/__tests__/template.test.ts

@@ -11,7 +11,7 @@ describe('server.getPreviewHeadHtml', () => {
  describe('when .storybook/preview-head.html does not exist', () => {
    beforeEach(() => {
      mock({
-        [`${__dirname}/../../templates/base-preview-head.html`]: BASE_HTML_CONTENTS,


So interesting... The FS mocking library makes pkg-dir sync command return undefined...

It's hacky, I know... But it does work...

ndelangen · 2022-01-26T13:55:11Z

lib/core-common/src/utils/template.ts

-    path.resolve(__dirname, '../templates/base-preview-body.html'),
-    'utf8'
-  );
+  const base = fs.readFileSync(`${sync(__dirname)}/templates/base-preview-body.html`, 'utf8');


Here's me using the pkg-dir sync command to get the fs location (directory) of package.json, to then concat that with where I know the template exists.

I did not use the @storybook/core-common......body.html here, because of the test. The mock util which I don't know very well, can't deal with that, and then node complains about not being able to resolve.

ndelangen · 2022-01-26T13:56:12Z

lib/core-server/package.json

@@ -31,8 +31,7 @@
  },
  "files": [
    "dist/**/*",
-    "dll/**/*",
-    "types/**/*",
+    "public/**/*",


We had a default favicon.ico in the source directory. I moved it to a public directory, so that we can stop using --copy-files flag from babel on this package.

ndelangen · 2022-01-26T13:57:04Z

lib/core-common/src/utils/template.ts

-    path.resolve(__dirname, '../templates/base-preview-head.html'),
-    'utf8'
-  );
+  const base = fs.readFileSync(`${sync(__dirname)}/templates/base-preview-head.html`, 'utf8');


Thinking about these templates, they really should be part of core-server, no?

I have no idea what the package naming means. What is core-common for precisely?

ndelangen · 2022-01-26T13:58:00Z

package.json

@@ -55,16 +55,11 @@
    "bootstrap": "node ./scripts/bootstrap.js",
    "build": "node ./scripts/build-package.js",
    "build-manager": "node -r esm ./scripts/build-manager.js",
-    "build-packs": "lerna exec --scope '@storybook/*' -- \\$LERNA_ROOT_PATH/scripts/build-pack.sh \\$LERNA_ROOT_PATH/packs",


I think we used this in the past before we had verdaccio?

I'm fairly sure this hasn't been used in years.

ndelangen · 2022-01-26T13:59:10Z

package.json

-    "dev": "concurrently --kill-others \"yarn dev:tsc\" \"yarn dev:babel\"",
-    "dev:babel": "lerna exec --scope \"@storybook/*\" --parallel -- cross-env-shell node \\$LERNA_ROOT_PATH/scripts/utils/watch-babel.js",
-    "dev:check-types": "tsc --noEmit",
-    "dev:tsc": "lerna exec --scope \"@storybook/*\" --parallel -- cross-env-shell node \\$LERNA_ROOT_PATH/scripts/utils/watch-tsc.js",


remove the dev commands.

The dev:check-types didn't work at ALL, it tried type checking stuff in node_modules.

ndelangen · 2022-01-26T14:00:01Z

package.json

+    "@types/react": "^16",
+    "@types/react-dom": "^16",


Just to ensure yarn doesn't get 'confused' and installs multiple version of this. We use version 16 in the monorepo.

ndelangen · 2022-01-26T14:03:34Z

scripts/bootstrap.js

+          `nx run-many --target="prepare" --all --parallel ${
            process.env.CI ? `--max-parallel=${maxConcurrentTasks}` : ''
-          }`
+          } -- --optimized`


I needed this in my prebundle work. It does nothing for now.. I guess i can remove it from this PR.

I struggled quite a bit to find a way to pass flags to the "target" from nx. There was no documentation on how to do this, but this works great 🎈

ndelangen · 2022-01-26T14:06:01Z

scripts/build-package.js

-      if (watchMode) {
-        const runWatchMode = () => {
-          const baseWatchCommand = `lerna exec --scope '${glob}' --parallel -- cross-env-shell node ${resolve(
-            __dirname
-          )}`;
-          const watchTsc = `${baseWatchCommand}/utils/watch-tsc.js`;
-          const watchBabel = `${baseWatchCommand}/utils/watch-babel.js`;
-          const command = `concurrently --kill-others-on-fail "${watchTsc}" "${watchBabel}"`;
-
-          spawn(command);
-        };
+  selection.filter(Boolean).forEach((v) => {
+    const sub = execa.command(`yarn prepare${watchMode ? ' --watch' : ''}`, {
+      cwd: resolve(__dirname, '..', v.location),
+      buffer: false,
+    });

-        runWatchMode();
-      } else {
-        spawn(`lerna run prepare --scope "${glob}"`);
-      }


So this has turned into a single case, the prepare script now can accept a --watch flag, and when it sees this the same code is re-used.

ndelangen · 2022-01-26T14:07:48Z

scripts/build-package.js

+    sub.stdout.on('data', (data) => {
+      process.stdout.write(`${chalk.cyan(v.name)}:\n${data}`);


prefix each chunk of output, with the package where it came from, make the name red, if it came from stdout.

ndelangen · 2022-01-26T14:08:49Z

scripts/build-package.js

+run().catch((e) => {
+  console.log(e);
+  process.exit(1);
+});


be able to do async-await in the script

ndelangen · 2022-01-26T14:09:31Z

scripts/prepare.js

+async function removeDist() {
+  await fs.remove('dist');


making things async-y so that hopefully we're blocking the main thread less, and we can do more with less resources.

ndelangen · 2022-01-26T14:10:51Z

scripts/prepare.js

+    babelify({
+      modules,
+      watch: flags.includes('--watch'),
+      errorCallback: (errorLogs) => logError('js', packageJson, errorLogs),
+    }),
+    tscfy({
+      watch: flags.includes('--watch'),
+      errorCallback: (errorLogs) => logError('ts', packageJson, errorLogs),
+    }),


Let's do them both in parallel, why not? their outputs might mingle, but the only output we're expecting are errors!

They currently still run in sub-processes anyway

ndelangen · 2022-01-26T15:17:32Z

scripts/build-package.js

-    stdio: 'pipe',
-  });
+const getStorybookPackages = async () => {
+  const { stdout } = await execa.command(`yarn workspaces list --json`);


not using lerna here anymore, perhaps over time we can remove it?

Seems like yarn workspaces & nx should be able to do everything lerna is doing?

ndelangen · 2022-01-26T15:35:05Z

scripts/run-registry.ts

@@ -69,7 +85,7 @@ const registryUrl = (command: string, url?: string) =>
  });


So I tried running this locally, and yarn 3 (which is what we're using) uses a different config property from previous versions of yarn. I'm not sure how it worked before.

ndelangen · 2022-01-26T15:35:49Z

scripts/run-registry.ts

+// const addUser = (url: string) =>
+//   new Promise((res, rej) => {
+//     logger.log(`👤 add temp user to verdaccio`);
+
+//     exec(`npx npm-cli-adduser -r "${url}" -a -u user -p password -e [email protected]`, (e) => {
+//       if (e) {
+//         rej(e);
+//       } else {
+//         res();
+//       }
+//     });
+//   });


I had to run this to get it to work locally, so I added this bit of code back. But it seems the CI doesn't need it, so commented it again.

ndelangen · 2022-01-26T15:38:31Z

scripts/utils/compile-tsc.js

-  const args = ['--outDir ./dist/ts3.9', '--listEmittedFiles true', '--declaration true'];
+  const args = [
+    '--outDir ./dist/ts3.9',
+    '--listEmittedFiles false',


No longer output the full list of files tsc outputted

ndelangen · 2022-01-26T15:50:24Z

tsconfig.json

+  "ts-node": {
+    "transpileOnly": true,
+    "files": true,
+    "compilerOptions": {
+      "types": [
+        "node"
+      ]
+    }
+  }


This should allow us to convert some more scripts over to TS, with negligible performance impact

ndelangen · 2022-01-26T15:51:06Z

scripts/utils/compile-tsc.js

+  if (!(await fs.pathExists(tsConfigFile))) {
    if (!silent) {
      console.log(`No ${tsConfigFile}`);
    }
    return;
  }

-  const content = fs.readFileSync(tsConfigFile);
-  const tsConfig = JSON.parse(content);
+  const tsConfig = await fs.readJSON(tsConfigFile);

-  if (tsConfig && tsConfig.lerna && tsConfig.lerna.disabled === true) {
-    if (!silent) {
-      console.log('Lerna disabled');
-    }
-    return;
+  if (!(tsConfig && tsConfig.lerna && tsConfig.lerna.disabled === true)) {
+    await run({ watch, silent, errorCallback });
  }

-  const command = getCommand(watch);
-  const { code, stderr } = shell.exec(command, { silent });
-
-  handleExit(code, stderr, errorCallback);
+  if (!watch) {
+    await execa.command('yarn run -T downlevel-dts dist/ts3.9 dist/ts3.4');
+  }


Make things more async-y to hopefully block the main thread less

ndelangen · 2022-01-26T15:56:09Z

scripts/utils/compile-babel.js

+  if (process.cwd().includes(path.join('lib', 'cli'))) {
+    args.push('--copy-files');
+  }


Only add the --copy-files flag for lib/cli.

Everywhere else, I've moved files to the root of the package and updated references; but in this package, it's quite hard to do. The generators are all none-source files, but they are currently placed in the src directory.

We've been talking about overhauling the lib/cli anyway: #1108
So it's OK if we leave this as-is for now.

tmeasday

Phew there is a lot in here. I don't really know this setup that well but nothing in here seems like a bad idea!

tmeasday · 2022-01-27T05:20:20Z

lib/core-common/src/utils/template.ts

-    path.resolve(__dirname, '../templates/base-preview-head.html'),
-    'utf8'
-  );
+  const base = fs.readFileSync(`${sync(__dirname)}/templates/base-preview-head.html`, 'utf8');


I have no idea what the package naming means. What is core-common for precisely?

ndelangen · 2022-01-27T08:11:30Z

core-common is a packages extracted from core to share the code that both builder-webpack4 and builder-webpack5 have in common. @shilman and I set it up when we introduced webpack 5 support

tmeasday · 2022-01-28T02:19:25Z

core-common is a packages extracted from core to share the code that both builder-webpack4 and builder-webpack5 have in common

I guess that's pretty confusing because "builders" related to the "server" modality, and yet there is another package called core-server 🤷

I guess when you ask "should this bit of core-common be in core-server?" I can't help but ask "shouldn't all of it?"

shilman · 2022-01-28T06:24:09Z

@ndelangen I updated this PR with:

a small documentation tweak
my changes to the versions.json/ts generator in CLI

I'm unable to review this PR (it crashes my browser), but I think it's ready to go. Waiting for CI results now.

yannbf · 2022-01-29T10:55:12Z

A bit late to the party, but I just wanted to say I tested this out on windows and it worked great! Took 25min to run a full bootstrap, and 20 seconds each prebundle step for the manager

ndelangen added 2 commits January 26, 2022 14:12

remove dev script

5d9a1c8

change the watch mode of yarn build to use the prepare script add a --watch flag to prepare script remove the babel --copy-file flag where possible remove noise from prepare improve error reporting from prepare cleanup

regen lockfile

cd36ab5