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.

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

CLI: Create sandbox on init in empty folders #23611

Closed
wants to merge 51 commits into from

Conversation

valentinpalkovic
Copy link
Contributor

@valentinpalkovic valentinpalkovic commented Jul 26, 2023

Closes N/A

What I did

This pull request refactors the CLI code to improve the sandbox generation and scaffolding features. It supports different package managers and nested dependency resolutions, using a new NestedDependencyResolution type and the overrides field in package.json. It also introduces a new scaffoldProject function that prompts the user to choose a template from the templates module during init, which contains updated and consistent configurations for various frameworks. Additionally, it adds a silent option to the sandbox command, which suppresses the console output, and a scaffolding-readme-template.md file, which is used to generate a custom README.md file for the scaffolded projects. Finally, it simplifies the project_types module and removes some unused files.

Walkthrough

🤖 Generated by Copilot at 969ec22

  • Implement new scaffolding feature for Storybook CLI (link,link,link,link,link,link)
  • Prompt user to select a template from a list of supported templates (link,link,link)
  • Clone template from sandbox repository and update package.json and README.md files (link,link,link)
  • Install dependencies and run Storybook if needed (link,link,link)
  • Filter out internal or in development templates for scaffolding (link,link)
  • Add silent option to suppress console output for sandbox command (link,link,link,link,link,link,link)
  • Add nested dependency resolution feature for different package managers (link,link,link,link,link,link,link,link,link,link,link,link,link,link,link)
  • Add more sandbox templates with different package managers (link,link,link,link)
  • Update template names to include bundler, language, and package manager information (link,link,link,link,link,link,link,link,link,link,link,link,link,link)
  • Update sandbox command to detect package manager and adjust init message accordingly (link)
  • Refactor project_types.ts file and avoid circular dependencies (link,link,link)
  • Fix registry URL detection for JsPackageManager class (link)
  • Simplify installDependencies method for JsPackageManager class (link)
  • Add assertNever helper function for type safety (link)
  • Delete unused functions and files (link,link,link)
  • Update sandbox generation workflows to use PNPM and disable YARN_ENABLE_IMMUTABLE_INSTALLS (link,link,link,link,link)
  • Add scaffolding-readme-template.md file as an entry point for ncc build command (link)
  • Fix formatting issue with welcome message (link)

How to test

  1. Run <storybook-project-folder>/code/lib/cli/bin/index.js init in an empty folder -> Storybook should automatically start
  2. Run <storybook-project-folder>/code/lib/cli/bin/index.js init --skip-install in an empty folder -> Storybook should not start automatically

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

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.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "build", "documentation", "maintenance", "dependencies", "other"]

🦋 Canary release

This pull request has been released as version 0.0.0-pr-23611-sha-d83da132. Install it by pinning all your Storybook dependencies to that version.

More information
Published version 0.0.0-pr-23611-sha-d83da132
Triggered by @yannbf
Repository storybookjs/storybook
Branch valentin/create-sandbox-in-empty-project
Commit d83da132
Datetime Fri Aug 11 05:49:47 UTC 2023 (1691732987)
Workflow run 5829362347

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=23611

@valentinpalkovic valentinpalkovic self-assigned this Jul 26, 2023
@valentinpalkovic valentinpalkovic changed the title Create sandbox on init in empty folders CLI: Create sandbox on init in empty folders Jul 26, 2023
@ndelangen
Copy link
Member

How does it determine which "selected template" with an empty directory?
Does it ask?

@valentinpalkovic
Copy link
Contributor Author

@ndelangen, The user will be asked which template should be chosen: https://github.com/storybookjs/storybook/pull/23611/files#diff-1be00149590160bd0f7412dcca41c8941da299807d73504bd06064ee6b2c9e0bR266-R282

@valentinpalkovic valentinpalkovic force-pushed the valentin/create-sandbox-in-empty-project branch from 71331c5 to 3be4580 Compare July 26, 2023 11:38
code/lib/cli/src/initiate.ts Outdated Show resolved Hide resolved
code/lib/cli/src/initiate.ts Outdated Show resolved Hide resolved
@yannbf
Copy link
Member

yannbf commented Jul 26, 2023

@valentinpalkovic we explicitly do a few things in the sandbox command, but in the case of users initializing Storybook in empty projects, it won't make much sense:

  • resolutions in package.json: It's so that repros are consistent, but for the init case it will just make things slow.
  • "before-storybook" in the name field in package.json
  • .stackblitzrc: I guess it won't hurt to have it, but doesn't make sense
  • README.md: It's made for sandboxes, mentioning that it's for reproductions and pointing people to a stackblitz link in the sandboxes repo

@valentinpalkovic
Copy link
Contributor Author

Good findings! I will tackle them

@valentinpalkovic valentinpalkovic force-pushed the valentin/create-sandbox-in-empty-project branch from 2d2f8b2 to 0c8cd76 Compare July 27, 2023 13:06
@@ -0,0 +1,23 @@
# {{template.name}}
Copy link
Member

Choose a reason for hiding this comment

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

Hey @kylegach you might be interested in this. This would be the README file for people who initialize Storybook in empty projects

@yannbf
Copy link
Member

yannbf commented Jul 27, 2023

A reminder, I'm not sure if it would be done in this PR or not, but this documentation section would need to be reworked:

<summary><code>storybook init</code> is not made for empty projects</summary>

@valentinpalkovic
Copy link
Contributor Author

@yannbf @jonniebigodes is working on the documentation. I had already a pairing session with him yesterday :)

@socket-security
Copy link

socket-security bot commented Jul 31, 2023

No top level dependency changes detected. Learn more about Socket for GitHub ↗︎

@valentinpalkovic valentinpalkovic force-pushed the valentin/create-sandbox-in-empty-project branch 3 times, most recently from 4f9788c to a474360 Compare August 3, 2023 12:07
Comment on lines +242 to +243
await remove(join(afterDir, '.yarn', 'cache'));
await remove(join(afterDir, '.yarn', 'unplugged'));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this actually already mentioned in the .gitignore file?

@vanessayuenn
Copy link
Contributor

Tested with npx and yarn; here are some findings:

❌ npx x react-vite

→ hangs on install dependencies step forever, no node_module was created.

❌ npx x angular

→ hangs on install dependencies step forever, no node_module was created.

✅ yarn x react-vite

✅ yarn x lit

❌ yarn x vue

→ failed at install dependencies with the esbuild error below, but i can actually start storybook straight away with yarn storybook.

postinstall error
# This file contains the result of Yarn building a package (esbuild@npm:0.18.19)
# Script name: postinstall

node:internal/errors:865
  const err = new Error(message);
              ^

Error: Command failed: /Users/spontaneouslyodd/.nvm/versions/node/v18.17.0/bin/node /Users/spontaneouslyodd/Projects/empty-vue-yarn/node_modules/esbuild/bin/esbuild --version
node:internal/errors:496
    ErrorCaptureStackTrace(err);
    ^

TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension "" for /Users/spontaneouslyodd/Projects/empty-vue-yarn/node_modules/esbuild/bin/esbuild
    at new NodeError (node:internal/errors:405:5)
    at Object.getFileProtocolModuleFormat [as file:] (node:internal/modules/esm/get_format:79:11)
    at defaultGetFormat (node:internal/modules/esm/get_format:124:36)
    at defaultLoad (node:internal/modules/esm/load:84:20)
    at nextLoad (node:internal/modules/esm/loader:163:28)
    at load$1 (file:///private/var/folders/2w/x2lktvm10wd005k_jp6vcpwr0000gn/T/xfs-3f5eb0ab/dlx-55309/.pnp.loader.mjs:1456:12)
    at nextLoad (node:internal/modules/esm/loader:163:28)
    at ESMLoader.load (node:internal/modules/esm/loader:603:26)
    at ESMLoader.moduleProvider (node:internal/modules/esm/loader:457:22)
    at new ModuleJob (node:internal/modules/esm/module_job:64:26) {
  code: 'ERR_UNKNOWN_FILE_EXTENSION'
}

Node.js v18.17.0

    at checkExecSyncError (node:child_process:885:11)
    at Object.execFileSync (node:child_process:921:15)
    at validateBinaryVersion (/Users/spontaneouslyodd/Projects/empty-vue-yarn/node_modules/esbuild/install.js:98:28)
    at /Users/spontaneouslyodd/Projects/empty-vue-yarn/node_modules/esbuild/install.js:283:5 {
  status: 1,
  signal: null,
  output: [
    null,
    Buffer(0) [Uint8Array] [],
    Buffer(998) [Uint8Array] [
      110, 111, 100, 101,  58, 105, 110, 116, 101, 114, 110,  97,
      108,  47, 101, 114, 114, 111, 114, 115,  58,  52,  57,  54,
       10,  32,  32,  32,  32,  69, 114, 114, 111, 114,  67,  97,
      112, 116, 117, 114, 101,  83, 116,  97,  99, 107,  84, 114,
       97,  99, 101,  40, 101, 114, 114,  41,  59,  10,  32,  32,
       32,  32,  94,  10,  10,  84, 121, 112, 101,  69, 114, 114,
      111, 114,  32,  91,  69,  82,  82,  95,  85,  78,  75,  78,
       79,  87,  78,  95,  70,  73,  76,  69,  95,  69,  88,  84,
       69,  78,  83,  73,
      ... 898 more items
    ]
  ],
  pid: 55440,
  stdout: Buffer(0) [Uint8Array] [],
  stderr: Buffer(998) [Uint8Array] [
    110, 111, 100, 101,  58, 105, 110, 116, 101, 114, 110,  97,
    108,  47, 101, 114, 114, 111, 114, 115,  58,  52,  57,  54,
     10,  32,  32,  32,  32,  69, 114, 114, 111, 114,  67,  97,
    112, 116, 117, 114, 101,  83, 116,  97,  99, 107,  84, 114,
     97,  99, 101,  40, 101, 114, 114,  41,  59,  10,  32,  32,
     32,  32,  94,  10,  10,  84, 121, 112, 101,  69, 114, 114,
    111, 114,  32,  91,  69,  82,  82,  95,  85,  78,  75,  78,
     79,  87,  78,  95,  70,  73,  76,  69,  95,  69,  88,  84,
     69,  78,  83,  73,
    ... 898 more items
  ]
}

Node.js v18.17.0

❌ yarn x angular

→ same thing as above -- failed at postinstall but i can actually start storybook straight away manually.

@yannbf yannbf force-pushed the valentin/create-sandbox-in-empty-project branch 2 times, most recently from 908903a to eaaa4a7 Compare August 11, 2023 10:42
@yannbf yannbf force-pushed the valentin/create-sandbox-in-empty-project branch from eaaa4a7 to fee58c9 Compare August 11, 2023 12:24
Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

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

only reviewed sandbox-templates.ts, this is great!

export type Cadence = keyof typeof templatesByCadence;

export type Template = {
/**
* Readable name for the template, which will be used for feedback and the status page
*/
name: string;
name: `${string} (${'Webpack5' | 'Vite'} | ${'JavaScript' | 'TypeScript'} | ${
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
name: `${string} (${'Webpack5' | 'Vite'} | ${'JavaScript' | 'TypeScript'} | ${
name: `${string} (${'Webpack' | 'Vite'} | ${'JavaScript' | 'TypeScript'} | ${

I don't think we need to explicitly mention Webpack version 5, given v4 is a long time ago and isn't supported.

},
'nextjs/default-js': {
name: 'Next.js (JavaScript)',
script: 'yarn create next-app {{beforeDir}} --javascript --eslint',
name: 'Next.js v13 (Webpack5 | JavaScript | npm)',
Copy link
Contributor

Choose a reason for hiding this comment

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

For all of these frameworks and renderers which just install the latest version, we should change the version string to be "Latest".
Otherwise it will be a pain to remember to update these strings all the time. I also think it's more correct.

},
'angular-cli/prerelease': {
name: 'Angular CLI (Prerelease)',
name: 'Angular v16 (Webpack5 | TypeScript | npm) (prerelease)',
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to my comment above, I think we should just call it prerelease.

Suggested change
name: 'Angular v16 (Webpack5 | TypeScript | npm) (prerelease)',
name: 'Angular Prerelease (Webpack5 | TypeScript | npm)',

@valentinpalkovic
Copy link
Contributor Author

I am closing this for now. At some point, we might start to work on it again.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants