Remove all ts-ignore in vue, tighten typings #11547

abrenneke · 2020-07-15T01:11:31Z

What I did

Well while learning how the Vue integration works for fixing unrelated issues I went down the typescript rabbit-hole and ended up improving the typings for Vue, and a little bit for all of the apps.

All @ts-ignore and most todo are removed
Reduced the amount of any in the code
Tightened the typings, for example StoryFnVueReturnType is now string | VueConstructor | VueOptions, where VueOptions is any argument that Vue.extend() accepts.
More edge cases revealed and accounted for, because of the more-correct typings.

Some of the changes (such as to @storybook/client-api and @storybook/core) leaked out of the changes here and into the other apps. Those changes enhance type-safety across the board, so in my opinion they're good to have. I'll add comments to point them out.

@backbone87 reviewed #7578 and I got a lot of insight from those comments so maybe they would want to look this over.

All tests, builds, & linting passing (on my machine at least).

How to test

Is this testable with Jest or Chromatic screenshots?
Existing tests cover the changes
Does this need a new example in the kitchen sink apps?
Nope
Does this need an update to the documentation?
Nope

abrenneke · 2020-07-15T01:13:33Z

lib/core/tsconfig.json

@@ -3,7 +3,6 @@
  "compilerOptions": {
    "rootDir": "./src"
  },
-  "include": ["src/**/*", "typings.d.ts"],
-  "exclude": ["src/**.test.ts"],
-  "files": ["./typings.d.ts"]


Seemed redundant since typings.d.ts was included with include already.

abrenneke · 2020-07-15T01:17:24Z

lib/core/client.d.ts

@@ -0,0 +1,2 @@
+import start from './dist/client/index';
+export = start;


Adding typings to client.js here caused all the apps to go from import { start } from '@storybook/core/client' to import client from '@storybook/core/client'.

However, the actual implementation of core/client.js, core/src/index.js, and core/src/preview/index.ts matches this, as core/src/client/preview/index.ts exports an object as default instead of exporting multiple functions. Therefore to match that, has to be default-imported in typescript.

core/src/client/preview/index.ts could be changed to a more correct module, but I'm not sure if there was a reason it was done the way it is right now.

I'm not sure to understand why import { start } from '@storybook/core/client' need to become import client from '@storybook/core/client'. I added locally this lib/core/client.d.ts and has no error with import { start } from '@storybook/core/client'.

Should start not be client instead as start is only a property of the exported object?

That's strange that it worked fine for you @gaetanmaisse - typescript was definitely erroring for me when I kept it as import { start }.

You're right that it works fine once it compiles down to CommonJS, as it's basically const { start } = require('@storybook/client-api') either way, but it's not valid in ES modules or typescript to import a single property of a default-exported object. Babel fudges this restriction a bit, when interfacing with CommonJS code.

For correct ES modules, we would need to have:

core/src/client/preview/index.ts

export { ClientApi, StoryStore, ConfigApi } from '@storybook/client-api'; export { toId } from '@storybook/csf'; export { default as start } from './start';

core/src/client/index.ts

export * from './preview'; export * from './preview/types';

core/client.d.ts

export * from './dist/client';

Which is definitely simpler, but I didn't know if there was a reason for the current way, so I didn't touch that.

👍 thanks for the clarification, I agree with you it will be way simpler/clearer with the solution in your comment. I had the same question about the reason of this kind of export in other parts of Storybook but I never had a strong answer, so it's surely a code style legacy.

I'll go ahead and switch to the new style and hope nothing breaks 😄

This is really bizzare, yarn build core works, yarn build react works, yarn build core api react vue works, but yarn build --all fails when building @storybook/react. Is there anything funky going on when I run --all @gaetanmaisse?

Hmm I would say no, but maybe 🙈 😂 Is it working with yarn bootstrap --core?

abrenneke · 2020-07-15T01:18:54Z

lib/client-api/src/types.ts

@@ -95,7 +95,7 @@ export interface GetStorybookKind {

 // This really belongs in lib/core, but that depends on lib/ui which (dev) depends on app/react
 // which needs this type. So we put it here to avoid the circular dependency problem.
-export type RenderContext = StoreItem & {
+export type RenderContext<StoryFnReturnType = unknown> = StoreItem<StoryFnReturnType> & {


This required some changes in the other apps besides Vue, but now they all get the correct type from calling storyFn() instead of any, so it's a win in my opinion. It's technically breaking if there are any other apps out there, that expect any instead of unknown? unknown is generally more type-safe though.

abrenneke · 2020-07-15T01:19:44Z

app/vue/tsconfig.json

-    "resolveJsonModule": true
+    "resolveJsonModule": true,
+    "strict": true,
+    "noImplicitReturns": true


Made the Vue app as strict as I could for typescript, to catch everything, can revert these if wanted.

abrenneke · 2020-07-15T01:20:58Z

app/vue/.eslintrc.js

+    {
+      files: ['*.ts', '*.tsx'],
+      rules: {
+        '@typescript-eslint/explicit-function-return-type': 'error',


Made the Vue app as strict as I could for typescript, to catch everything, can revert these if wanted.

abrenneke · 2020-07-15T01:22:55Z

app/vue/src/client/preview/util.ts

    // known limitation: we don't have the component instance to pass
-    return def.call();
+    return (validator.default as () => T | null | undefined)();


Tried as I might, I couldn't get this to be completely type-safe using a type guard. I think this would need negated types to remove T & Function from the possible types.

abrenneke · 2020-07-15T01:24:09Z

app/vue/src/client/preview/types.ts

-// TODO: some vue expert needs to look at this
-export type StoryFnVueReturnType = string | Component;
+/** Any options that can be passed into Vue.extend */
+export type VueOptions = Exclude<Parameters<VueConstructor['extend']>[0], undefined>;


I could not find a union of all options interfaces in the Vue typings, pulling all possible arguments to extend gets them all quite easily. The types are very long otherwise. Let me know if you know a better type.

abrenneke · 2020-07-15T01:25:05Z

app/server/src/client/index.ts

@@ -5,7 +5,6 @@ export {
  addParameters,
  configure,
  getStorybook,
-  forceReRender,


I'm guessing this was an old field - not present any more once type-safety was added.

abrenneke · 2020-07-15T01:26:01Z

app/aurelia/src/client/preview/render.ts

  const element = storyFn();

  if (!element) {
    showError({
-      title: `Expecting an Aurelia component from the story: "${selectedStory}" of "${selectedKind}".`,
+      title: `Expecting an Aurelia component from the story.`,


Newer way to get the story name? I didn't look too far into it.

abrenneke · 2020-07-15T01:28:36Z

package.json

@@ -240,7 +240,7 @@
    "ts-dedent": "^1.1.1",
    "ts-jest": "^26.1.0",
    "ts-node": "^8.9.1",
-    "typescript": "^3.4.0",
+    "typescript": "^3.7.0",


Typescript 3.9 is already pinned in the yarn.lock, but since I used optional chaining which requires at least typescript 3.7, I figured this should be changed

I think you can set ^3.9.6 directly because as you said we are already using it

Fair enough!

@gaetanmaisse it looks like there are a lot of duplicates in yarn.lock (like, typescript shows up 15 times in yarn why typescript), it might be a good idea to run yarn-deduplicate to clean that up.

True! I plan to do a big dep clean and dedup work in Storybook 6.1 :)

abrenneke · 2020-07-15T01:32:40Z

app/vue/src/client/preview/index.ts


+function prepare(rawStory: undefined, innerStory?: VueConstructor): undefined;


A little bit awkward, but maintains the type-safety effectively, since prepare will only return undefined if rawStory === undefined, which won't happen if someone uses typescript, but may happen otherwise.

abrenneke · 2020-07-15T01:33:36Z

app/server/src/client/preview/index.ts

@@ -28,7 +28,7 @@ const setRenderFetchAndConfigure: ClientApi['configure'] = (loader, module, opti
  if (options && options.fetchStoryHtml) {
    setFetchStoryHtml(options.fetchStoryHtml);
  }
-  api.configure(loader, module, framework);
+  api.configure(framework, loader, module);


This appeared to be just wrong? Was revealed by the type-safety

shilman · 2020-07-15T01:38:34Z

@abrenneke This looks amazing, great work!!! @backbone87 can you please take a look?

Less any, more explicit types and type checks Turned on noImplicitReturns, strict, explicit-function-return-type (vue)

gaetanmaisse · 2020-07-17T20:12:16Z

@abrenneke I will take a second look asap 😉

abrenneke requested review from alterx, igor-dv, kazupon, shilman, thomasbertet and tmeasday as code owners July 15, 2020 01:11

abrenneke commented Jul 15, 2020

View reviewed changes

abrenneke force-pushed the vue-typescript-safety branch from a985e88 to 64373e2 Compare July 15, 2020 01:27

abrenneke commented Jul 15, 2020

View reviewed changes

shilman added vue bug typescript labels Jul 15, 2020

remove all ts-ignore in vue, tighten typings

49f4a3a

Less any, more explicit types and type checks Turned on noImplicitReturns, strict, explicit-function-return-type (vue)

abrenneke force-pushed the vue-typescript-safety branch from 64373e2 to 49f4a3a Compare July 15, 2020 01:39

Pin typescript to 3.9.6

6aa02ef

abrenneke requested a review from gaetanmaisse July 17, 2020 19:48

abrenneke mentioned this pull request Jul 17, 2020

Vue automatic controls, required dependencies #11600

Closed

4 tasks

shilman added the in progress label Jul 31, 2020

shilman added this to the 6.1 core milestone Jul 31, 2020

shilman modified the milestones: 6.1 core, 6.2 core Nov 4, 2020

abrenneke closed this Feb 10, 2021

This was referenced Dec 15, 2021

[Snyk] Security upgrade react-dev-utils from 9.1.0 to 12.0.0 Watemlifts/storybook#273

Closed

[Snyk] Security upgrade react-dev-utils from 9.1.0 to 12.0.0 Watemlifts/storybook#274

Closed

This was referenced Apr 7, 2022

[Snyk] Security upgrade react-scripts from 3.3.0 to 5.0.0 Watemlifts/storybook#296

Closed

[Snyk] Security upgrade react-scripts from 3.4.4 to 5.0.0 Watemlifts/storybook#297

Closed

[Snyk] Fix for 14 vulnerabilities Watemlifts/storybook#298

Closed

snyk-bot mentioned this pull request May 25, 2022

[Snyk] Security upgrade react-scripts from 3.3.0 to 5.0.0 Watemlifts/storybook#310

Open

This was referenced Jun 2, 2022

[Snyk] Security upgrade react-scripts from 3.4.4 to 5.0.0 Watemlifts/storybook#311

Open

[Snyk] Security upgrade react-scripts from 3.3.0 to 5.0.0 Watemlifts/storybook#314

Open

snyk-bot mentioned this pull request Mar 19, 2023

[Snyk] Fix for 5 vulnerabilities lili2311/storybook#106

Open

lili2311 mentioned this pull request Jun 23, 2023

[Snyk-dev] Fix for 1 vulnerabilities lili2311/storybook#131

Open

lili2311 mentioned this pull request Aug 11, 2023

[Snyk-dev] Fix for 12 vulnerabilities lili2311/storybook#144

Open

This was referenced Sep 25, 2023

[Snyk-dev] Fix for 3 vulnerabilities lili2311/storybook#167

Open

[Snyk-dev] Fix for 3 vulnerabilities lili2311/storybook#170

Open

This was referenced Nov 25, 2023

[Snyk-dev] Fix for 23 vulnerabilities lili2311/storybook#221

Open

[Snyk-dev] Fix for 11 vulnerabilities lili2311/storybook#223

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove all ts-ignore in vue, tighten typings #11547

Remove all ts-ignore in vue, tighten typings #11547

abrenneke commented Jul 15, 2020

abrenneke Jul 15, 2020 •

edited

Loading

abrenneke Jul 15, 2020 •

edited

Loading

gaetanmaisse Jul 15, 2020

abrenneke Jul 16, 2020 •

edited

Loading

gaetanmaisse Jul 17, 2020

abrenneke Jul 17, 2020

abrenneke Jul 24, 2020

gaetanmaisse Jul 27, 2020

abrenneke Jul 15, 2020

abrenneke Jul 15, 2020

abrenneke Jul 15, 2020

abrenneke Jul 15, 2020

abrenneke Jul 15, 2020

abrenneke Jul 15, 2020

abrenneke Jul 15, 2020

abrenneke Jul 15, 2020

gaetanmaisse Jul 15, 2020

abrenneke Jul 16, 2020

gaetanmaisse Jul 17, 2020

abrenneke Jul 15, 2020

abrenneke Jul 15, 2020

shilman commented Jul 15, 2020

gaetanmaisse commented Jul 17, 2020

		@@ -0,0 +1,2 @@
		import start from './dist/client/index';
		export = start;


		function prepare(rawStory: undefined, innerStory?: VueConstructor): undefined;

Remove all ts-ignore in vue, tighten typings #11547

Remove all ts-ignore in vue, tighten typings #11547

Conversation

abrenneke commented Jul 15, 2020

What I did

How to test

abrenneke Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

abrenneke Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abrenneke Jul 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shilman commented Jul 15, 2020

gaetanmaisse commented Jul 17, 2020

abrenneke Jul 15, 2020 •

edited

Loading

abrenneke Jul 15, 2020 •

edited

Loading

abrenneke Jul 16, 2020 •

edited

Loading