Build: Migrate @storybook/scripts to strict-ts

[stilt0n]

Part of #22176

What I did

Migrate @storybook/scripts to strict-ts

More specifically:

• Changed scripts tsconfig.json to use strict types
• Created a typechecking script for @storybook/scripts based on the one in prepare/check.ts
• Resolved type errors that came up after enabling strict types
• Adds check:scripts to CI

How to test

Check types with cd scripts && yarn check. The typecheck script should run in CI now as well.

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"]

[stilt0n]

I'm not sure what err might be here so this seemed safest. But if it's expected always to be an Error it might be better to make writeJunitXml take unknown and use an invariant similar to what sendTelemetryError does in #23632 .

[kasperpeulen]

Nice! Do we also run the new task in circle CI? @stilt0n

[stilt0n]

@kasperpeulen I haven't added it to the CI, but I can see if I can add it later today.

I only took a short look, but is the CI just running yarn task --task check --no-link --start-from=install or something similar? In that case I would just need to get nx to recognize scripts as a project included in --all?

[kasperpeulen]

@stilt0n I think so.
I wonder what @tmeasday, @yannbf or @ndelangen think about that.

[stilt0n]

With the "nx" field these can be picked up by nx run and nx run-many. Unfortunately, if we try to do this all in one command, the check task would also run check-package which I think we don't want it to do.

I considered renaming check to check:package, but then I would need to know everywhere it is used so it can be renamed, which is tricky using just text search.

[stilt0n]

Here we could avoid using two commands with:

'nx run-many --target="check","check:scripts" --all <...>'

But I think we'd need to rename the "check" script in code/package.json to make that work.

[stilt0n]

I've added a commit to make the new typechecking script run on CI. I've left a few comments on parts where I think it might be able to be improved. I think the current solution ought to work as is, but I'd be interested in suggestions on how to improve it.

[kasperpeulen]

@stilt0n Discussed this with @yannbf
He thinks that it is actually better to do something similar as we have done for:

storybook/.circleci/config.yml

Lines 186 to 201 in 5c3c839

	script-unit-tests:
	executor: sb_node_16_browsers
	steps:
	- git-shallow-clone/checkout_advanced:
	clone_options: '--depth 1 --verbose'
	- attach_workspace:
	at: .
	- run:
	name: Test
	command: |
	cd scripts
	yarn test --coverage --ci
	- store_test_results:
	path: scripts/junit.xml
	- report-workflow-on-failure
	- cancel-workflow-on-failure

As the scripts directory is kind of outside of the monorepo.

Maybe we could just run it before the tets:

  script-unit-tests: <-- maybe rename to script-ci-tests
    executor: sb_node_16_browsers
    steps:
      - git-shallow-clone/checkout_advanced:
          clone_options: '--depth 1 --verbose'
      - attach_workspace:
          at: .
      - run:
          name: Type check <-- new command
          command: |
            cd scripts
            yarn check
      - run:
          name: Test
          command: |
            cd scripts
            yarn test --coverage --ci
      - store_test_results:
          path: scripts/junit.xml
      - report-workflow-on-failure
      - cancel-workflow-on-failure

[stilt0n]

@kasperpeulen Thanks for the suggestion! That seems much better than what I have.

[kasperpeulen]

@stilt0n Thanks! I will do a proper review tomorrow.

[stilt0n]

I don't really think it makes a lot of sense to keep this "test" but if it were really important to prevent these particular cases from being changed this could be wrapped in a trivial test to silence compiler warnings, e.g.

describe('TS "tests"', () => {
  it('MaybeOptionValues and OptionValues type test', () => {
    // deepscan-disable-next-line
    function test(mv: MaybeOptionValues<typeof allOptions>, v: OptionValues<typeof allOptions>) {
      console.log(mv.first, mv.second, mv.third, mv.fourth, mv.fifth, mv.sixth);
      // @ts-expect-error as it's not allowed
      console.log(mv.seventh);
      console.log(v.first, v.second, v.third, v.fourth, v.fifth, v.sixth);
      // @ts-expect-error as it's not allowed
      console.log(v.seventh);
    }
    expect(test).toBeDefined();
  });
});

[kasperpeulen]

What about using https://github.com/mmkal/expect-type?
We also use that in https://github.com/storybookjs/storybook/blob/e7acb0ed4e5e1bed8274bd53070e753680bba692/code/renderers/react/src/public-types.test.tsx

[kasperpeulen]

I don't think this test matters that much indeed, so I'm just gonna merge it thanks!

[kasperpeulen]

@stilt0n I changed the getErrorMessage utility to using tiny-invariant instead. I think that is slightly better, as we want to assume it is an error, but TS (or JS really) cannot give that guarantee.

[kasperpeulen]

LGTM!