Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
Jan 28, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5d9a1c8
remove dev script
ndelangen Jan 26, 2022
cd36ab5
regen lockfile
ndelangen Jan 26, 2022
5444107
cleanup
ndelangen Jan 26, 2022
e4aff5c
cleanup
ndelangen Jan 26, 2022
bea3416
Fix remaining yarn dev reference
shilman Jan 28, 2022
9c77ed4
Merge branch 'next' into tech/improve-dev-babel-setup
shilman Jan 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .babelrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }],
Copy link
Member Author

Choose a reason for hiding this comment

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

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. 🎈

Copy link
Member Author

Choose a reason for hiding this comment

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

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

'@babel/plugin-proposal-export-default-from',
'@babel/plugin-syntax-dynamic-import',
['@babel/plugin-proposal-object-rest-spread', { loose: true, useBuiltIns: true }],
Expand Down
1 change: 0 additions & 1 deletion .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

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

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

command: yarn test:e2e-framework vue3 angular130 angular13 angular12 angular11 web_components_typescript web_components_lit2
no_output_timeout: 5m
- store_artifacts:
Expand Down
1 change: 1 addition & 0 deletions addons/docs/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
"common/**/*",
"ember/**/*",
"html/**/*",
"svelte/**/*",
Copy link
Member Author

Choose a reason for hiding this comment

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

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?

"postinstall/**/*",
"react/**/*",
"vue/**/*",
Expand Down
4 changes: 2 additions & 2 deletions addons/docs/src/frameworks/svelte/prepareForInline.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ import { AnyFramework, StoryFn } from '@storybook/csf';

import React from 'react';

// @ts-ignore
import HOC from './HOC.svelte';
Comment on lines -5 to -6
Copy link
Member Author

Choose a reason for hiding this comment

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

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?

// eslint-disable-next-line import/no-extraneous-dependencies
import HOC from '@storybook/addon-docs/svelte/HOC.svelte';

export const prepareForInline = (storyFn: StoryFn<AnyFramework>) => {
const el = React.useRef(null);
Expand Down
1 change: 1 addition & 0 deletions addons/docs/src/typings.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ declare module 'require-from-string';
declare module 'styled-components';
declare module 'acorn-jsx';
declare module 'vue/dist/vue';
declare module '@storybook/addon-docs/svelte/HOC.svelte';

declare module 'sveltedoc-parser' {
export function parse(options: any): Promise<any>;
Expand Down
0 addons/docs/src/frameworks/svelte/HOC.svelte → addons/docs/svelte/HOC.svelte
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion addons/storysource/src/register.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import React from 'react';
import { addons } from '@storybook/addons';

import { StoryPanel } from './StoryPanel';
import { ADDON_ID, PANEL_ID } from '.';
Copy link
Member Author

Choose a reason for hiding this comment

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

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?

import { ADDON_ID, PANEL_ID } from './index';

addons.register(ADDON_ID, (api) => {
addons.addPanel(PANEL_ID, {
Expand Down
3 changes: 2 additions & 1 deletion app/svelte/src/client/preview/decorators.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { DecoratorFunction, StoryContext, LegacyStoryFn } from '@storybook/csf';
import { sanitizeStoryContextUpdate } from '@storybook/store';
import SlotDecorator from './SlotDecorator.svelte';
// eslint-disable-next-line import/no-extraneous-dependencies
import SlotDecorator from '@storybook/svelte/templates/SlotDecorator.svelte';
import { SvelteFramework } from './types';

/**
Expand Down
16 changes: 7 additions & 9 deletions app/svelte/src/client/preview/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,17 @@ import { decorateStory } from './decorators';
import './globals';
import { render, renderToDOM } from './render';

const { configure: coreConfigure, clientApi, forceReRender } = start(renderToDOM, {
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;
Comment on lines +7 to +17
Copy link
Member Author

Choose a reason for hiding this comment

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

just formatting


const framework = 'svelte';
export const storiesOf = (kind: string, m: any) =>
Expand Down
5 changes: 3 additions & 2 deletions app/svelte/src/client/preview/render.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import global from 'global';
import { ArgsStoryFn } from '@storybook/csf';
import { RenderContext } from '@storybook/store';
import type { RenderContext } from '@storybook/store';
// eslint-disable-next-line import/no-extraneous-dependencies
import PreviewRender from '@storybook/svelte/templates/PreviewRender.svelte';
import { SvelteFramework } from './types';
import PreviewRender from './PreviewRender.svelte';

const { document } = global;

Expand Down
3 changes: 2 additions & 1 deletion app/svelte/src/typings.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
declare module 'global';
declare module '*.svelte';
declare module '@storybook/svelte/templates/SlotDecorator.svelte';
declare module '@storybook/svelte/templates/PreviewRender.svelte';
0 ...e/src/client/preview/PreviewRender.svelte → app/svelte/templates/PreviewRender.svelte
View file
Open in desktop
File renamed without changes.
0 ...e/src/client/preview/SlotDecorator.svelte → app/svelte/templates/SlotDecorator.svelte
View file
Open in desktop
File renamed without changes.
4 changes: 2 additions & 2 deletions docs/contribute/code.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,8 @@ npx sb@next link --local /path/to/local-repro-directory

<details>

<summary>`yarn dev` watches everything but is resource-intensive</summary>
Copy link
Member Author

Choose a reason for hiding this comment

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

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

<summary>`yarn build --all --watch` watches everything but is resource-intensive</summary>

It's troublesome to know which packages you're going to change ahead of time, and watching all of them can be highly demanding, even on modern machines. If you're working on a powerful enough machine, you can use `yarn dev` instead of `yarn build`.
It's troublesome to know which packages you're going to change ahead of time, and watching all of them can be highly demanding, even on modern machines. If you're working on a powerful enough machine, you can use `yarn build --all --watch` instead of `yarn build`.

</details>
3 changes: 1 addition & 2 deletions lib/builder-webpack4/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@
},
"files": [
"dist/**/*",
"dll/**/*",
"types/**/*",
Comment on lines -34 to -35
Copy link
Member Author

Choose a reason for hiding this comment

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

cleanup, neither of these folders exist anymore

"templates/**/*",
Copy link
Member Author

Choose a reason for hiding this comment

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

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!

"*.js",
"*.d.ts"
],
Expand Down
6 changes: 5 additions & 1 deletion lib/builder-webpack4/src/preview/iframe-webpack.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,11 @@ export default async (options: Options & Record<string, any>): Promise<Configura

const configEntryPath = path.resolve(path.join(workingDir, 'storybook-config-entry.js'));
virtualModuleMapping[configEntryPath] = handlebars(
await readTemplate(path.join(__dirname, 'virtualModuleModernEntry.js.handlebars')),
await readTemplate(
require.resolve(
'@storybook/builder-webpack4/templates/virtualModuleModernEntry.js.handlebars'
Copy link
Member Author

Choose a reason for hiding this comment

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

So here's a discussion point:

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

  1. How I've done it now: refer to the whole npm package.
  2. 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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

)
),
{
storiesFilename,
configs,
Expand Down
0 ...ew/virtualModuleModernEntry.js.handlebars → ...es/virtualModuleModernEntry.js.handlebars
View file
Open in desktop
File renamed without changes.
3 changes: 1 addition & 2 deletions lib/builder-webpack5/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@
},
"files": [
"dist/**/*",
"dll/**/*",
"types/**/*",
Comment on lines -34 to -35
Copy link
Member Author

Choose a reason for hiding this comment

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

cleanup

"templates/**/*",
"*.js",
"*.d.ts"
],
Expand Down
6 changes: 5 additions & 1 deletion lib/builder-webpack5/src/preview/iframe-webpack.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,11 @@ export default async (options: Options & Record<string, any>): Promise<Configura
virtualModuleMapping[storiesPath] = toImportFn(stories);
const configEntryPath = path.resolve(path.join(workingDir, 'storybook-config-entry.js'));
virtualModuleMapping[configEntryPath] = handlebars(
await readTemplate(path.join(__dirname, 'virtualModuleModernEntry.js.handlebars')),
await readTemplate(
require.resolve(
'@storybook/builder-webpack5/templates/virtualModuleModernEntry.js.handlebars'
)
),
{
storiesFilename,
configs,
Expand Down
0 ...ew/virtualModuleModernEntry.js.handlebars → ...es/virtualModuleModernEntry.js.handlebars
View file
Open in desktop
File renamed without changes.
3 changes: 3 additions & 0 deletions lib/channel-websocket/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
"compilerOptions": {
"rootDir": "./src"
},
"include": [
"src/**/*"
],
Comment on lines +6 to +8
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

"exclude": [
"src/**/*.test.*",
"src/**/tests/**/*",
Expand Down
3 changes: 1 addition & 2 deletions lib/cli/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,7 @@
"dist/**/*",
"README.md",
"*.js",
"*.d.ts",
"versions.json"
Copy link
Member Author

@ndelangen ndelangen Jan 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

cleanup: This file does not exist

"*.d.ts"
],
"scripts": {
"prepare": "node ../../scripts/prepare.js",
Expand Down
13 changes: 9 additions & 4 deletions lib/cli/scripts/generate-sb-packages-versions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
#!/usr/bin/env node

const { writeJson, readJson } = require('fs-extra');
const { readJson, writeFile } = require('fs-extra');
const path = require('path');
const globby = require('globby');
const semver = require('@storybook/semver');
const { default: dedent } = require('ts-dedent');

const rootDirectory = path.join(__dirname, '..', '..', '..');

Expand Down Expand Up @@ -39,9 +40,13 @@ const run = async () => {
.sort((package1, package2) => package1.name.localeCompare(package2.name))
.reduce((acc, { name }) => ({ ...acc, [name]: updatedVersion }), {});

await writeJson(path.join(__dirname, '..', 'src', 'versions.json'), packageToVersionMap, {
spaces: 2,
});
await writeFile(
path.join(__dirname, '..', 'src', 'versions.ts'),
dedent`
// auto generated file, do not edit
export default ${JSON.stringify(packageToVersionMap, null, 2)}
`
);
};
Comment on lines +43 to 50
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Screenshot 2022-01-26 at 14 47 24

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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


run().catch((e) => {
Expand Down
2 changes: 1 addition & 1 deletion lib/cli/src/js-package-manager/JsPackageManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { sync as spawnSync } from 'cross-spawn';
import { commandLog } from '../helpers';
import { PackageJson, PackageJsonWithDepsAndDevDeps } from './PackageJson';
import { readPackageJson, writePackageJson } from './PackageJsonHelper';
import storybookPackagesVersions from '../versions.json';
import storybookPackagesVersions from '../versions';

const logger = console;

Expand Down
2 changes: 1 addition & 1 deletion lib/cli/src/js-package-manager/NPMProxy.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ describe('NPM Proxy', () => {
describe('getVersion', () => {
it('with a Storybook package listed in versions.json it returns the version', async () => {
// eslint-disable-next-line global-require
const storybookAngularVersion = require('../versions.json')['@storybook/angular'];
const storybookAngularVersion = require('../versions').default['@storybook/angular'];
const executeCommandSpy = jest.spyOn(npmProxy, 'executeCommand').mockReturnValue('"5.3.19"');

const version = await npmProxy.getVersion('@storybook/angular');
Expand Down
57 changes: 0 additions & 57 deletions lib/cli/src/versions.json
View file
Open in desktop

This file was deleted.

58 changes: 58 additions & 0 deletions lib/cli/src/versions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
// auto generated file, do not edit
Copy link
Member Author

Choose a reason for hiding this comment

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

@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?

export default {
'@storybook/addon-a11y': '6.5.0-alpha.23',
'@storybook/addon-actions': '6.5.0-alpha.23',
'@storybook/addon-backgrounds': '6.5.0-alpha.23',
'@storybook/addon-controls': '6.5.0-alpha.23',
'@storybook/addon-docs': '6.5.0-alpha.23',
'@storybook/addon-essentials': '6.5.0-alpha.23',
'@storybook/addon-interactions': '6.5.0-alpha.23',
'@storybook/addon-jest': '6.5.0-alpha.23',
'@storybook/addon-links': '6.5.0-alpha.23',
'@storybook/addon-measure': '6.5.0-alpha.23',
'@storybook/addon-outline': '6.5.0-alpha.23',
'@storybook/addon-storyshots': '6.5.0-alpha.23',
'@storybook/addon-storyshots-puppeteer': '6.5.0-alpha.23',
'@storybook/addon-storysource': '6.5.0-alpha.23',
'@storybook/addon-toolbars': '6.5.0-alpha.23',
'@storybook/addon-viewport': '6.5.0-alpha.23',
'@storybook/addons': '6.5.0-alpha.23',
'@storybook/angular': '6.5.0-alpha.23',
'@storybook/api': '6.5.0-alpha.23',
'@storybook/builder-webpack4': '6.5.0-alpha.23',
'@storybook/builder-webpack5': '6.5.0-alpha.23',
'@storybook/channel-postmessage': '6.5.0-alpha.23',
'@storybook/channel-websocket': '6.5.0-alpha.23',
'@storybook/channels': '6.5.0-alpha.23',
'@storybook/cli': '6.5.0-alpha.23',
'@storybook/client-api': '6.5.0-alpha.23',
'@storybook/client-logger': '6.5.0-alpha.23',
'@storybook/codemod': '6.5.0-alpha.23',
'@storybook/components': '6.5.0-alpha.23',
'@storybook/core': '6.5.0-alpha.23',
'@storybook/core-client': '6.5.0-alpha.23',
'@storybook/core-common': '6.5.0-alpha.23',
'@storybook/core-events': '6.5.0-alpha.23',
'@storybook/core-server': '6.5.0-alpha.23',
'@storybook/csf-tools': '6.5.0-alpha.23',
'@storybook/ember': '6.5.0-alpha.23',
'@storybook/html': '6.5.0-alpha.23',
'@storybook/instrumenter': '6.5.0-alpha.23',
'@storybook/manager-webpack4': '6.5.0-alpha.23',
'@storybook/manager-webpack5': '6.5.0-alpha.23',
'@storybook/node-logger': '6.5.0-alpha.23',
'@storybook/postinstall': '6.5.0-alpha.23',
'@storybook/preact': '6.5.0-alpha.23',
'@storybook/preview-web': '6.5.0-alpha.23',
'@storybook/react': '6.5.0-alpha.23',
'@storybook/router': '6.5.0-alpha.23',
'@storybook/server': '6.5.0-alpha.23',
'@storybook/source-loader': '6.5.0-alpha.23',
'@storybook/store': '6.5.0-alpha.23',
'@storybook/svelte': '6.5.0-alpha.23',
'@storybook/theming': '6.5.0-alpha.23',
'@storybook/ui': '6.5.0-alpha.23',
'@storybook/vue': '6.5.0-alpha.23',
'@storybook/vue3': '6.5.0-alpha.23',
'@storybook/web-components': '6.5.0-alpha.23',
};
2 changes: 1 addition & 1 deletion lib/components/src/blocks/Story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import global from 'global';
import React, { createElement, ElementType, FunctionComponent, Fragment } from 'react';

import type { Parameters } from '@storybook/api';
Copy link
Member Author

Choose a reason for hiding this comment

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

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

import type { Parameters } from '@storybook/csf';

import { IFrame } from './IFrame';
import { EmptyBlock } from './EmptyBlock';
Expand Down
2 changes: 1 addition & 1 deletion lib/core-common/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@
},
"files": [
"dist/**/*",
"dll/**/*",
Copy link
Member Author

Choose a reason for hiding this comment

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

cleanup

"types/**/*",
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

"templates/**/*",
"*.js",
"*.d.ts"
],
Expand Down
Loading