Create a new yarn example command that will drive our new dev experience

[tmeasday]

Telescoping on #18759

Issue: https://linear.app/chromaui/issue/SB-629/create-the-framework-to-allow-the-command-to-run

What I did

• Add options and cli-step libraries to ease the creation of scripts that take options either on the CLI or via prompt, and ultimately run yarn storybook <x> with various commands.

• Use that to sketch out the example script.

Screen.Recording.2022-07-25.at.10.41.04.pm.mov

How to test

Run yarn example.

Open questions

1. What should we do when the example already exists? This will likely be the norm.
   a. Should we blow it away and start again?
   b. Should we try and reinstall dependencies?
   c. Should we just leave it? (what if its configuration is different to the specified one, eg. different addons)?

• answer: store a "configuration" alongside the example, and compare to the one the user is using.
  • If identical, continue (with a warning).
  • If different, prompt for deletion / continue w/o touching.
  • Allow flags to allow force deletion / force continue.

2. Why does the current e2e script sometimes call npx -p @storybook/cli sb repro rather than always running sb from the current code base? cc @gaetanmaisse

• answer: Create a new yarn example command that will drive our new dev experience #18781 (comment) -- yarn shenanigans. @ndelangen and @yannbf have encountered this and worked around it in the template generators.

TODOs

In this PR

• Add a dry-run option, allow exec to be passed dryRun
• Refactor cli commands to be callable directly
• Add flags for existing

In followup PRs

• Use degit rather than creating new repros (by default)
• Figure out linking
• Fix sb add <addon>
• Copy stories (once they exist) from addons + core (in react).
• Call sb serve (once it is added)
• Call yarn dev (once it is added)
• Store configuration and check

[gaetanmaisse]

Why does the current e2e script sometimes call npx -p @storybook/cli sb repro rather than always running sb from the current code base? cc @gaetanmaisse

For bad historical reasons related to yarn/npm base path if I remember correctly. We should change a bit the code of the CLI to be able to provide the path to use for each yarn/npm command executed by the CLI itself, something here to handle cwd option I think: https://github.com/storybookjs/storybook/blob/next/lib/cli/src/js-package-manager/JsPackageManager.ts#L284

[tmeasday]

@ndelangen the unit tests are failing as I added some tests for utils inside scripts/, which is outside the jest root (code/). Should I:

1. Change the jest root
2. Move the util code inside code somewhere (where?)
3. Move just the tests inside code somewhere (sort of weird but could work)?
4. Add a second jest env for scripts/?

[ndelangen]

@tmeasday I think adding a jest config for scripts is the right course of action.

[yannbf]

Nice stuff @tmeasday 😍 I am adding a few comments as I try it out

[yannbf]

@tmeasday the script should probably handle exiting the command mid-way.


I pressed ctrl + C and it kept going:

[yannbf]

<3

[kasperpeulen]

Sadly, to have it inferred, you either need a contextual type or as const indeed. You don't have to do it on the object, you can also do it on multiple: true as const, which seems fine to me.

A contextual type like satisfies OptionSpecifier in options.test.ts may be ideal, but only in a future version of TS 😅 . Otherwise it needs a hard annotation const allOptions: OptionSpecifier or the contextual type can be provided using a factory function.

[tmeasday]

I'm pretty open to whatever folks prefer here, so I will definitely be guided by you here @kasperpeulen. What's interesting about how it's done here is that in the source file you don't have to do anything special apart from pass the options in as a raw object (i.e you don't need to type it or use as const):

storybook/scripts/example.ts

Lines 15 to 58 in 457714d

	return getOptionsOrPrompt('yarn example', {
	framework: {
	description: 'Which framework would you like to use?',
	values: frameworks,
	required: true,
	},
	addon: {
	description: 'Which extra addons (beyond the CLI defaults) would you like installed?',
	values: addons,
	multiple: true,
	},
	includeStories: {
	description: "Include Storybook's own stories?",
	promptType: (_, { framework }) => framework === 'react',
	},
	create: {
	description: 'Create the example from scratch (rather than degitting it)?',
	},
	forceDelete: {
	description: 'Always delete an existing example, even if it has the same configuration?',
	promptType: false,
	},
	forceReuse: {
	description: 'Always reuse an existing example, even if it has a different configuration?',
	promptType: false,
	},
	link: {
	description: 'Link the storybook to the local code?',
	inverse: true,
	},
	start: {
	description: 'Start the example Storybook?',
	inverse: true,
	},
	build: {
	description: 'Build the example Storybook?',
	},
	watch: {
	description: 'Start building used packages in watch mode as well as the example Storybook?',
	},
	dryRun: {
	description: "Don't execute commands, just list them (dry run)?",
	},
	});

That seems good because there's less possibility of messing it up. Then again, with this stuff what does "messing it up" mean? It means that the object you get back out of the call (to say getOptionsOrPrompt) will have the wrong type. Which I guess will be pretty obvious, and pretty quickly fixed.

---

The other thing I am wondering folks' option on (cc @yannbf etc) is the use of these fields to imply the type of the option. i.e. using:

const booleanOption = {};
const stringOption = { values: ['a'] }; // the presence of `values` implies this is a string option.

As opposed to:

const booleanOption = { type: 'boolean' as const };
const stringOption = { type: 'string' as const, values: ['a'] };

here it is much more explicit, but also wordier.

I guess the major downside of the current "implied" approach is if say down the track we want to get rid of the restriction that StringOptions must have an array of values, we might have an issue. But then again we can always address that if/when it happens (it wouldn't be that hard to update the codebase and add a type field to all the options).

[kasperpeulen]

I think that the true as const on multiple is only needed in places where there is no contextual type, so for example here:

storybook/scripts/utils/options.test.ts

Lines 12 to 30 in 93f3e0e

	const allOptions = {
	first: {
	description: 'first',
	},
	second: {
	description: 'second',
	inverse: true,
	},
	third: {
	description: 'third',
	values: ['one', 'two', 'three'],
	required: true as const,
	},
	fourth: {
	description: 'fourth',
	values: ['a', 'b', 'c'],
	multiple: true as const,
	},
	};

But in example.ts it should not be needed, as the arguments of the function call are contextually typed:

storybook/scripts/example.ts

Lines 18 to 29 in 93f3e0e

	async function getOptions() {
	return getOptionsOrPrompt('yarn example', {
	framework: {
	description: 'Which framework would you like to use?',
	values: frameworks,
	required: true as const,
	},
	addon: {
	description: 'Which extra addons (beyond the CLI defaults) would you like installed?',
	values: addons,
	multiple: true as const,
	},

I checked that with or without the as const, the type inferred is properly, and the as string[] here is not needed anymore 😄 :

storybook/scripts/example.ts

Lines 180 to 184 in 93f3e0e

	for (const addon of optionValues.addon as string[]) {
	const addonName = `@storybook/addon-${addon}`;
	// eslint-disable-next-line no-await-in-loop
	await executeCLIStep(steps.add, { argument: addonName, cwd, dryRun });
	}

[kasperpeulen]

@yannbf @tmeasday

About the structure implying the type. I think it is considered best practice to always have a clear discriminant in unions (they are also often called "discriminated unions", "tagged unions"). But I don't know, because of TS flexbility it is more and more pushed in directions that is quite unexplored in other languages with those kind of features 😅.

One downside of this approach is that you need type predicates to keep it readable:

storybook/scripts/utils/options.ts

Lines 79 to 89 in 93f3e0e

	export function isStringOption(option: Option): option is StringOption {
	return 'values' in option && !('multiple' in option);
	}
	export function isBooleanOption(option: Option): option is BooleanOption {
	return !('values' in option);
	}
	export function isStringArrayOption(option: Option): option is StringArrayOption {
	return 'values' in option && 'multiple' in option;
	}

And although they are pretty awesome, they are also not typesafe, in the sense that TS won't check at all if you predicate is correct.

You could make a discriminant type?: 'string' | 'array' and undefined implying it being a boolean, to make it shorter.

I was also thinking that, if this features grows, you might consider using zod for this. It is excellent in specifying runtime types, inferring the TS type from that, and also adding extra validation on top of that. It is often used in combination with form libraries, and I guess this is pretty much a form 😅.

It could look like this:

async function getOptions() {
  return getOptionsOrPrompt('yarn example', {
    framework: {
      type: z.enum(['react', 'angular']),
      description: 'Which framework would you like to use?',
    },
    addon: {
      type: z.array(z.enum(['a11y', 'storysource'])),
      description: 'Which extra addons (beyond the CLI defaults) would you like installed?',
    },
    create: {
      // default type may be z.boolean.default(false)
      description: 'Create the example from scratch (rather than degitting it)?',
    },
    start: {
      // inversed
      type: z.boolean().default(true),
      description: 'Start the example Storybook?',
    },
    email: {
      // all kind of helpers for free
      type: z.string().email(),
      description: 'Age',
    },
    age: {
      type: z.number().int().min(0),
      description: 'Age',
    },

[tmeasday]

Yeah, cool. I think I'll stop swimming upstream and just use the discriminated union here.

I was also thinking that, if this features grows, you might consider using zod for this

So yeah, I think zod is definitely conceptually in the same headspace as what I was doing here (love "parse, don't validate"!).

I guess the tricky part here is that zod is a parser/validator but what we want to parse/validate is CLI flags and commander is already doing that for us. So I'm not exactly sure how easy it will be to extract all the information we need to give to commander from zod. I'll have a play with it! It's definitely better than coming up with some new API for .string().email() etc ;)

[tmeasday]

@kasperpeulen one quick question about zod -- if we were going to do { type: z.something() } how would we then do runtime branching based on zod type? For instance, to pick the correct prompt to show the user if they haven't filled out an option?

Previously we used the isStringOption() and friends to do that, is there a way to do that, considering that things like z.string().default() can end up with multiple layers of wrapped validators?

[kasperpeulen]

@tmeasday

Yes, I do love zod parse don't validate as well :D I like to make sure all incoming data is handled at the very edges of the program, so that you can really trust TS inside the application, parse it right away and fail fast if needed.

I think it should be possible to get all the information needed for commander and prompt with runtime branching. You can unwrap the default by accessing _def.innerType. Possibly, recursively, but to start we could maybe constrain the type a bit, like this:

type OptionType =
  | z.ZodDefault<z.ZodBoolean | z.ZodEnum<[string, ...string[]]>>
  | z.ZodBoolean
  | z.ZodEnum<[string, ...string[]]>
  | z.ZodArray<z.ZodEnum<[string, ...string[]]>>;

function toPromptType(option: {
  key: string;
  type: OptionType;
  description: string;
}): PromptObject {
  const { type, key, description } = option;
  if (type instanceof z.ZodDefault) {
    return {
      ...toPromptType({ ...option, type: type._def.innerType }),
      initial: type._def.defaultValue(),
    };
  }
  if (type instanceof z.ZodBoolean) {
    return { type: 'toggle', name: key, message: description };
  }
  if (type instanceof z.ZodArray) {
    return {
      name: key,
      type: 'autocompleteMultiselect',
      choices: type.element.options.map((it) => ({ title: it, value: it })),
    };
  }
  return {
    name: key,
    type: 'select',
    choices: type.options.map((it) => ({ title: it, value: it })),
  };
}

[tmeasday]

Ok, got it! I thought that was probably a way to go but I guess I was hoping there was something simpler I was missing ;)

[kasperpeulen]

Hehe, yeah, it is certainly not zod biggest strength. But you do get type inference for free with this approach (which is a strength of zod):

Btw, zod only works well when the tsconfig is set to strict: true. I couldn't get the infer to work if I didn't change the tsconfig.

[tmeasday]

Yes that's nice! I got something similar going last night in our homegrown system too: