CLI: Add "missing-storybook-dependencies" automigration #28579

yannbf · 2024-07-12T15:55:48Z

Relates to #28566

What I did

This PR introduces a new automigration that checks whether the user is utilizing Storybook APIs from packages that are not directly installed, and then installs them as a solution.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

stories
unit tests
integration tests
end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Add imports to consolidated packages which are not installed in package.json: @storybook/preview-api, @storybook/manager-api, @storybook/theming for instance
Make sure to add import references to these packages in files under .storybook
Make sure to add import references to these packages in story files
Run the automigrate command, see whether things are reported
Make sure that upon saying 'yes', the automigration installs things as intended and the Storybook packages are consistent.

Documentation

Add or update documentation reflecting your changes
If you are deprecating/removing a feature, make sure to update
MIGRATION.MD

Checklist for Maintainers

When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts
Make sure this PR contains one of the labels below:
Available labels
- bug: Internal changes that fixes incorrect behavior.
- maintenance: User-facing maintenance tasks.
- dependencies: Upgrading (sometimes downgrading) dependencies.
- build: Internal-facing build tooling & test updates. Will not show up in release changelog.
- cleanup: Minor cleanup style change. Will not show up in release changelog.
- documentation: Documentation only changes. Will not show up in release changelog.
- feature request: Introducing a new feature.
- BREAKING CHANGE: Changes that break compatibility in some way with current major version.
- other: Changes that don't fit in the above categories.

🦋 Canary release

This pull request has been released as version 0.0.0-pr-28579-sha-08603e88. Try it out in a new sandbox by running npx storybook@0.0.0-pr-28579-sha-08603e88 sandbox or in an existing project with npx storybook@0.0.0-pr-28579-sha-08603e88 upgrade.

More information


Published version	`0.0.0-pr-28579-sha-08603e88`
Triggered by	@yannbf
Repository	storybookjs/storybook
Branch	`yann/missing-storybook-deps-automigration`
Commit	`08603e88`
Datetime	Fri Jul 12 17:03:43 UTC 2024 (`1720803823`)
Workflow run	9911485188

To request a new release of this pull request, mention the @storybookjs/core team.

core team members can create a new canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=28579

name	before	after	diff	%
createTime	6.4s	23s	16.5s	🔺71.9%
generateTime	20.7s	20.7s	20ms	0.1%
initTime	20s	20.9s	923ms	4.4%
createSize	0 B	0 B	0 B	-
generateSize	76.5 MB	76.5 MB	0 B	0%
initSize	198 MB	198 MB	-538 kB	-0.3%
diffSize	122 MB	121 MB	-538 kB	-0.4%
buildTime	13.6s	13.5s	-86ms	-0.6%
buildSize	7.59 MB	7.59 MB	0 B	0%
buildSbAddonsSize	1.63 MB	1.63 MB	0 B	0%
buildSbCommonSize	195 kB	195 kB	0 B	0%
buildSbManagerSize	2.3 MB	2.3 MB	0 B	0%
buildSbPreviewSize	349 kB	349 kB	0 B	0%
buildStaticSize	0 B	0 B	0 B	-
buildPrebuildSize	4.47 MB	4.47 MB	0 B	0%
buildPreviewSize	3.12 MB	3.12 MB	0 B	0%
testBuildTime	0ms	0ms	0ms	-
testBuildSize	0 B	0 B	0 B	-
testBuildSbAddonsSize	0 B	0 B	0 B	-
testBuildSbCommonSize	0 B	0 B	0 B	-
testBuildSbManagerSize	0 B	0 B	0 B	-
testBuildSbPreviewSize	0 B	0 B	0 B	-
testBuildStaticSize	0 B	0 B	0 B	-
testBuildPrebuildSize	0 B	0 B	0 B	-
testBuildPreviewSize	0 B	0 B	0 B	-
devPreviewResponsive	7.6s	7.8s	180ms	2.3%
devManagerResponsive	5.1s	5.1s	-15ms	-0.3%
devManagerHeaderVisible	790ms	723ms	-67ms	🔰-9.3%
devManagerIndexVisible	815ms	749ms	-66ms	🔰-8.8%
devStoryVisibleUncached	1.1s	1.1s	19ms	1.7%
devStoryVisible	833ms	776ms	-57ms	🔰-7.3%
devAutodocsVisible	710ms	702ms	-8ms	-1.1%
devMDXVisible	773ms	640ms	-133ms	🔰-20.8%
buildManagerHeaderVisible	758ms	698ms	-60ms	🔰-8.6%
buildManagerIndexVisible	761ms	700ms	-61ms	🔰-8.7%
buildStoryVisible	808ms	748ms	-60ms	🔰-8%
buildAutodocsVisible	684ms	625ms	-59ms	🔰-9.4%
buildMDXVisible	663ms	638ms	-25ms	-3.9%

yannbf · 2024-07-12T15:58:51Z

code/core/src/common/js-package-manager/NPMProxy.ts

@@ -132,28 +132,28 @@ export class NPMProxy extends JsPackageManager {
    });
  }

-  public async findInstallations(pattern: string[]) {


The idea of adding a depth option is because we want to try and get information over dependencies that are coming directly from package.json, but NOT as dependencies of dependencies. This needs further testing on all package managers

yannbf · 2024-07-12T15:59:36Z

code/lib/cli/src/automigrate/fixes/missing-storybook-dependencies.ts

+       * So we first install the exact version, then run code again
+       * to write to package.json to add the caret back, but without running install
+       */
+      await packageManager.addDependencies(


This really sucks, I hope the comment at the top explains why it's needed

nx-cloud · 2024-07-12T16:00:29Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit e0a8106. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 1 target

nx run-many -t build --parallel=3

Sent with 💌 from NxCloud.

greptile-apps

PR Summary

Added depth parameter to findInstallations in /code/core/src/common/js-package-manager/JsPackageManager.ts
Enhanced findInstallations in /code/core/src/common/js-package-manager/NPMProxy.ts with depth parameter
Modified findInstallations in /code/core/src/common/js-package-manager/PNPMProxy.ts to accept depth parameter
Updated findInstallations in /code/core/src/common/js-package-manager/Yarn1Proxy.ts and /code/core/src/common/js-package-manager/Yarn2Proxy.ts with depth parameter
Introduced missingStorybookDependencies automigration in /code/lib/cli/src/automigrate/fixes/index.ts and added corresponding tests in /code/lib/cli/src/automigrate/fixes/missing-storybook-dependencies.test.ts

_{8 file(s) reviewed, 5 comment(s)}
_{Edit PR Review Bot Settings}

code/lib/cli/src/automigrate/fixes/missing-storybook-dependencies.test.ts

code/lib/cli/src/automigrate/fixes/missing-storybook-dependencies.ts

greptile-apps · 2024-07-12T16:00:07Z

code/lib/cli/src/automigrate/fixes/missing-storybook-dependencies.ts

+      const content = await readFile(file, 'utf-8');
+      dependenciesToCheck.forEach((pkg) => {
+        // match imports like @storybook/theming or @storybook/theming/create
+        const regex = new RegExp(`['"]${pkg}(/[^'"]*)?['"]`);


🪶 style: The regex pattern could be more specific to avoid false positives.

greptile-apps · 2024-07-12T16:00:08Z

code/lib/cli/src/automigrate/fixes/missing-storybook-dependencies.ts

+    `;
+  },
+
+  async run({ result: { packageUsage }, dryRun, packageManager }) {


🪶 style: Consider adding a log statement for when the dry run is enabled to inform the user that no changes will be made.

yannbf · 2024-07-12T16:44:30Z

code/lib/cli/src/automigrate/fixes/missing-storybook-dependencies.ts

+  // the dependency is directly installed, else they could come from other dependencies
+  const promises = packages.map((pkg) => packageManager.findInstallations([pkg], { depth: 0 }));
+
+  const analyses = await Promise.all(promises);


This unfortunately adds several seconds in the check, in contrast with just passing the entire array as a pattern. The reason I do this is because it's possible that in yarn, if you pass a package that does not exist, an error will be thrown. As a result, you get undefined and then lose the information regarding all packages which is not great. Iterating through the packages instead fixes that, at the cost of slower times because then it does multiple requests instead.

HOWEVER it takes long mostly in Yarn 1, which is really slow in nature. For the other package managers it's actually OK (2-3s)

How about we change it so the slow implementation is only in yarn1?

So we keep passing the array to packageManager.findInstallations but then inside the yarn1 implementation we do the Promise.all and split it over multiple processes?

That way all the other package managers perf are unaffected.

The Promise.all solution was for both yarn 1 and 2! I didn’t test on npm and pnpm to know whether it would work there as well

I see. 👍

ndelangen · 2024-07-12T19:07:48Z

@yannbf What do you think about this test failing:
https://github.com/storybookjs/storybook/actions/runs/9912469422/job/27387368973?pr=28579
?

yannbf · 2024-07-14T10:47:03Z

code/lib/cli/src/automigrate/fixes/missing-storybook-dependencies.ts

+      const packageJson = await packageManager.retrievePackageJson();
+      await packageManager.addDependencies(
+        { installAsDevDependencies: true, skipInstall: true, packageJson },
+        dependenciesToInstall.map((pkg) => `${pkg}@${versionToInstall}`)


we need to add a test to make sure that if the user has the modifiers (or lack thereof), they're kept:

~8.1.0

^8.1.0

8.1.0

shilman

Tested in a few different scenarios & package managers. LGTM!

…s-automigration CLI: Add "missing-storybook-dependencies" automigration (cherry picked from commit 8dc43db)

yannbf added 3 commits July 12, 2024 17:32

add "missing-storybook-dependencies" automigration

b8209a0

second pass, use shallow depth in checks

780f1b9

update error message

24215f0

yannbf added bug cli ci:normal labels Jul 12, 2024

yannbf commented Jul 12, 2024

View reviewed changes

greptile-apps bot reviewed Jul 12, 2024

View reviewed changes

yannbf added 2 commits July 12, 2024 18:36

fix vitest imports

23ddfff

add specific version ranges

08603e8

yannbf commented Jul 12, 2024

View reviewed changes

Merge branch 'next' into yann/missing-storybook-deps-automigration

9e923f0

ndelangen added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Jul 12, 2024

ndelangen assigned yannbf and ndelangen Jul 12, 2024

ndelangen added ci:normal and removed ci:normal labels Jul 12, 2024

ndelangen added 2 commits July 12, 2024 22:58

fix ts error

c9ea814

fix tests

c269e84

ndelangen mentioned this pull request Jul 12, 2024

Upgrade: Install/Add Storybook dependencies the default way #28572

Draft

8 tasks

ndelangen added 3 commits July 12, 2024 23:42

create consistent reporting (seems there sort ordering race condition)

8467592

Merge branch 'next' into yann/missing-storybook-deps-automigration

939eedc

chalk not putting in colors in CI causing snapshot differences

68d17f1

ndelangen approved these changes Jul 12, 2024

View reviewed changes

Merge branch 'next' into yann/missing-storybook-deps-automigration

e0a8106

yannbf commented Jul 14, 2024

View reviewed changes

shilman approved these changes Jul 15, 2024

View reviewed changes

shilman merged commit 8dc43db into next Jul 15, 2024
52 of 53 checks passed

shilman deleted the yann/missing-storybook-deps-automigration branch July 15, 2024 08:57

shilman added the automigrations label Jul 15, 2024

github-actions bot mentioned this pull request Jul 15, 2024

Release: Prerelease 8.3.0-alpha.1 #28509

Merged

33 tasks

storybook-bot pushed a commit that referenced this pull request Jul 15, 2024

Merge pull request #28579 from storybookjs/yann/missing-storybook-dep…

48286d0

…s-automigration CLI: Add "missing-storybook-dependencies" automigration (cherry picked from commit 8dc43db)

github-actions bot mentioned this pull request Jul 15, 2024

Release: Patch 8.2.3 #28564

Merged

13 tasks

github-actions bot added the patch:done Patch/release PRs already cherry-picked to main/release branch label Jul 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI: Add "missing-storybook-dependencies" automigration #28579

CLI: Add "missing-storybook-dependencies" automigration #28579

yannbf commented Jul 12, 2024 •

edited

Loading

yannbf Jul 12, 2024

yannbf Jul 12, 2024

nx-cloud bot commented Jul 12, 2024 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot Jul 12, 2024

greptile-apps bot Jul 12, 2024

yannbf Jul 12, 2024 •

edited

Loading

ndelangen Jul 12, 2024

yannbf Jul 12, 2024

ndelangen Jul 12, 2024

ndelangen commented Jul 12, 2024

yannbf Jul 14, 2024

shilman left a comment

CLI: Add "missing-storybook-dependencies" automigration #28579

CLI: Add "missing-storybook-dependencies" automigration #28579

Conversation

yannbf commented Jul 12, 2024 • edited Loading

What I did

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

Manual testing

Documentation

Checklist for Maintainers

🦋 Canary release

yannbf Jul 12, 2024

Choose a reason for hiding this comment

yannbf Jul 12, 2024

Choose a reason for hiding this comment

nx-cloud bot commented Jul 12, 2024 • edited Loading

☁️ Nx Cloud Report

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Jul 12, 2024

Choose a reason for hiding this comment

greptile-apps bot Jul 12, 2024

Choose a reason for hiding this comment

yannbf Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

ndelangen Jul 12, 2024

Choose a reason for hiding this comment

yannbf Jul 12, 2024

Choose a reason for hiding this comment

ndelangen Jul 12, 2024

Choose a reason for hiding this comment

ndelangen commented Jul 12, 2024

yannbf Jul 14, 2024

Choose a reason for hiding this comment

shilman left a comment

Choose a reason for hiding this comment

yannbf commented Jul 12, 2024 •

edited

Loading

nx-cloud bot commented Jul 12, 2024 •

edited

Loading

yannbf Jul 12, 2024 •

edited

Loading