Tech: improve global types #20184

ndelangen · 2022-12-09T14:12:21Z

Issue:

Closes: #19386

What I did

strong typing for globals
fix issues resulting from strong global types

I ran into this global package not having any types, and got mad at it, so I created https://github.com/ndelangen/global
Now global types are not any, and reflect the globalThis.

The issue is vite doesn't pass certain files throw the transform plugin, this causes the globalization to not be complete, causing multiple version of the same module, which is designed to be a singleton. me and Ian looked at it, we need to investigate further. Co-authored-by: Ian VanSchooten <[email protected]>

…pes, enforce clientApi singleton via global

ndelangen · 2022-12-09T22:22:44Z

I need to verify this actually fixes the vite dev issue

…torev6

…-mode-with-storystorev6

…torev6

IanVS · 2022-12-12T17:31:25Z

I'd love to find some time to finish up #19386, to give proper types for globals.

…ned' into norbert/sb-1063-vite-in-dev-mode-with-storystorev6

…torev6

…torev6-cleaned

…torev6

…ned' into norbert/sb-1063-vite-in-dev-mode-with-storystorev6

ndelangen · 2022-12-13T09:26:42Z

@IanVS I think this PR replaces that one

IanVS

It's a bummer there's so much duplicated work here between this and #19386, and it would be nice to avoid the use of a dependency at all, but I see the benefit to easier mocking this way, and we get some nice wins on the correct typing, which is the main reason I wanted to replace global in the first place.

I think we could probably be more rigorous on the typing of our own globals, but as I said elsewhere, I think that can come later.

I had a few small suggestions, but this looks like a good step to take.

IanVS · 2022-12-13T14:48:48Z

code/addons/a11y/src/typings.d.ts

@@ -1 +1,2 @@
-declare module 'global';
+/* eslint-disable @typescript-eslint/naming-convention */
+declare var __STORYBOOK_STORY_STORE__: any;


Shouldn't this be something like StoryStore<Renderer>?

In my PR to remove the global dependency, I took a crack at defining the types for storybook globals here: https://github.com/storybookjs/storybook/pull/19386/files#diff-c945b52732b89295a5b35f8ed082c713dd8002a29420b607bfc382672e203fe0R28

Maybe you're planning to deal with these types in a separate PR, which would probably make sense since this is already a large PR.

Yeah it should be. considering this is in an addon, I didn't want to pull in, types from 'far away' packages. The PR is complex as is, and this seemed to me like a good candidate for a follow-up

IanVS · 2022-12-13T14:52:47Z

code/ui/components/src/syntaxhighlighter/syntaxhighlighter.tsx

@@ -56,8 +56,8 @@ export function createCopyToClipboardFunction() {
    return (text: string) => navigator.clipboard.writeText(text);
  }
  return async (text: string) => {
-    const tmp = document.createElement('TEXTAREA');
-    const focus = document.activeElement;
+    const tmp = document.createElement('TEXTAREA') as HTMLTextAreaElement;


Suggested change

const tmp = document.createElement('TEXTAREA') as HTMLTextAreaElement;

const tmp = document.createElement('textarea');

Any reason to keep this capitalized like this? It's what's preventing TS from knowing how to correctly type it.

IanVS · 2022-12-13T15:01:59Z

code/addons/storyshots/storyshots-core/src/frameworks/configure.ts

@@ -11,7 +11,7 @@ import type {
 import { toRequireContext } from '@storybook/core-webpack';
 import { normalizeStoriesEntry } from '@storybook/core-common';
 import registerRequireContextHook from '@storybook/babel-plugin-require-context-hook/register';
-import global from 'global';
+import { global } from '@storybook/global';


Hmmm, why isn't line 80 erroring below? I had to add a @ts-expect-error in my PR: https://github.com/storybookjs/storybook/pull/19386/files#diff-e76d868bbea9b46f39c61e743ff5bcc0d53f8c76e51c0ed1b7aef5e2fa1c3daaR78

good question, I don't know.. it's any:

code/lib/channel-postmessage/src/index.ts

code/lib/client-logger/src/typings.d.ts

IanVS · 2022-12-13T15:20:14Z

code/lib/client-logger/src/index.ts


-const { LOGLEVEL, console } = global;
+const { LOGLEVEL } = global;

 type LogLevel = 'trace' | 'debug' | 'info' | 'warn' | 'error' | 'silent';


Ow I see what you're doing here.. I'm afraid this will likely break the TS bundling, because the typings.d.ts loading feature isn't automatic.

We'd have to add a tripple-lash directive to make that work. Perhaps we can do the reverse though? We should be able to import the type of LogLevel in the typings.d.ts file? That way we have a single source of truth.

IanVS · 2022-12-13T15:28:56Z

code/lib/preview-api/src/modules/core-client/start.test.ts

@@ -106,6 +112,12 @@ function makeRequireContext(importMap: Record<Path, ModuleExports>) {
 describe('start', () => {
  beforeEach(() => {
    global.DOCS_OPTIONS = { enabled: false };
+    // @ts-expect-error (setting this to undefined is indeed what we want to do)


Is this an indication that global.__STORYBOOK_CLIENT_API__ is typed incorrectly? I think nearly all globals should be allowed to be undefined, since they will be undefined until they are set somewhere.

TypeScript doesn't like it:

reason being:

storybook/code/lib/preview-api/src/typings.d.ts

Lines 27 to 28 in adb0d25

var __STORYBOOK_CLIENT_API__: import('./modules/client-api/ClientApi').ClientApi<any>;

var __STORYBOOK_PREVIEW__: import('./modules/preview-web/PreviewWeb').PreviewWeb<any>;

I could set this to | undefined, sure... Do you think that's better?

I could set this to | undefined, sure... Do you think that's better?

I think so. That's what I did in my PR, to indicate that these values aren't always there, they have to be set during runtime, which means there's a period of time in which they are undefined. But maybe that's being overly pedantic.

IanVS · 2022-12-13T15:36:59Z

Issues aren't enabled in your fork, so I'll ask this here. Why is window preferred over globalThis, when we're asserting that the type is globalThis?

https://github.com/ndelangen/global/blob/5e592be1b6feaa00eb1a2ed1cd1c464a16eec907/src/index.ts#L4-L7

ndelangen · 2022-12-13T16:17:59Z

I copy-pasted that code from the original global package @IanVS
I didn't change much to it.
I can change it to prefer globalThis, I think.

…torev6

…js.handlebars

ndelangen and others added 6 commits December 1, 2022 18:33

some fixes, need more work

1925182

try to make builder-manager prefer mjs, for tree-shaking

f8d5551

unsupported temp api that's not supposed to be there, doesn't need types

db36af9

Merge branch 'next' into fix/storystorev6-vite

18d3b8d

strong typing for globals, fix issues resulting from strong global ty…

501dce1

…pes, enforce clientApi singleton via global

ndelangen self-assigned this Dec 9, 2022

ndelangen added the bug label Dec 9, 2022

ndelangen changed the title ~~strong typing for globals, fix issues resulting from strong global types, enforce clientApi singleton via global~~ Fix: enforce clientApi singleton via global Dec 9, 2022

ndelangen added 10 commits December 9, 2022 16:38

add "dom.iterable" and use global as window

42bf76e

fix all the global typings

3659bab

fix start test

e5b6648

fixing typings issues

f526092

fixing tests

a94e7e8

fixes

5be6d25

add new storybook scoped package to verdaccio config

141dec1

fix references to global in template dirs

3342776

fixes

43aca90

cleanup

d9f736a

ndelangen added 7 commits December 12, 2022 14:01

Merge branch 'next' into norbert/sb-1063-vite-in-dev-mode-with-storys…

5653ec8

…torev6

Merge branch 'fix/storystorev6-vite' into norbert/sb-1063-vite-in-dev…

3eb16e7

…-mode-with-storystorev6

Merge branch 'next' into norbert/sb-1063-vite-in-dev-mode-with-storys…

556e9e2

…torev6

simplify globals

b041209

fix

0761c15

fix

c1a62a2

ensure singleton via global

1263ac8

ndelangen mentioned this pull request Dec 12, 2022

Fix: vite devmode with storyStoreV6 by ensuring singleton via global #20207

Merged

reset the global every time, to get a new version

a7c357a

ndelangen changed the base branch from next to norbert/sb-1063-vite-in-dev-mode-with-storystorev6-cleaned December 12, 2022 17:53

ndelangen added 4 commits December 12, 2022 19:24

Merge branch 'norbert/sb-1063-vite-in-dev-mode-with-storystorev6-clea…

b052066

…ned' into norbert/sb-1063-vite-in-dev-mode-with-storystorev6

remove the window as global definition

b0977b8

Merge branch 'next' into norbert/sb-1063-vite-in-dev-mode-with-storys…

fe9b5d6

…torev6

Merge branch 'next' into norbert/sb-1063-vite-in-dev-mode-with-storys…

91fe4d8

…torev6-cleaned

ndelangen changed the title ~~Fix: enforce clientApi singleton via global~~ Tech: improve global types Dec 13, 2022

ndelangen added 4 commits December 13, 2022 09:49

Merge branch 'next' into norbert/sb-1063-vite-in-dev-mode-with-storys…

6a578c8

…torev6

fix

19f3ceb

Merge branch 'norbert/sb-1063-vite-in-dev-mode-with-storystorev6-clea…

adb0d25

…ned' into norbert/sb-1063-vite-in-dev-mode-with-storystorev6

cleanup

4f3b666

ndelangen force-pushed the norbert/sb-1063-vite-in-dev-mode-with-storystorev6-cleaned branch from 91fe4d8 to 775f3ae Compare December 13, 2022 11:19

IanVS approved these changes Dec 13, 2022

View reviewed changes

Base automatically changed from norbert/sb-1063-vite-in-dev-mode-with-storystorev6-cleaned to next December 13, 2022 16:41

ndelangen added 8 commits December 13, 2022 19:13

use feature detection

a088aa4

Merge branch 'next' into norbert/sb-1063-vite-in-dev-mode-with-storys…

a033d29

…torev6

Merge branch 'next' into norbert/sb-1063-vite-in-dev-mode-with-storys…

d7fb0c3

…torev6

Merge branch 'next' into norbert/sb-1063-vite-in-dev-mode-with-storys…

9da5823

…torev6

Merge branch 'next' into norbert/sb-1063-vite-in-dev-mode-with-storys…

7c2ccbb

…torev6

Merge branch 'next' into norbert/sb-1063-vite-in-dev-mode-with-storys…

88f5931

…torev6

Restore code/lib/builder-webpack5/templates/virtualModuleModernEntry.…

fc357d7

…js.handlebars

change to use @storybook/global in modern entry

bb2e680

ndelangen merged commit d772d38 into next Dec 20, 2022

ndelangen deleted the norbert/sb-1063-vite-in-dev-mode-with-storystorev6 branch December 20, 2022 12:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tech: improve global types #20184

Tech: improve global types #20184

ndelangen commented Dec 9, 2022 •

edited by IanVS

Loading

ndelangen commented Dec 9, 2022

IanVS commented Dec 12, 2022

ndelangen commented Dec 13, 2022

IanVS left a comment

IanVS Dec 13, 2022

ndelangen Dec 13, 2022

IanVS Dec 13, 2022

IanVS Dec 20, 2022

IanVS Dec 13, 2022

ndelangen Dec 13, 2022

IanVS Dec 13, 2022

ndelangen Dec 13, 2022

IanVS Dec 13, 2022

ndelangen Dec 13, 2022

IanVS Dec 13, 2022

IanVS commented Dec 13, 2022

ndelangen commented Dec 13, 2022

	const tmp = document.createElement('TEXTAREA') as HTMLTextAreaElement;
	const tmp = document.createElement('textarea');

	type LogLevel = 'trace' \| 'debug' \| 'info' \| 'warn' \| 'error' \| 'silent';
	type LogLevel = typeof LOGLEVEL;

	var __STORYBOOK_CLIENT_API__: import('./modules/client-api/ClientApi').ClientApi<any>;
	var __STORYBOOK_PREVIEW__: import('./modules/preview-web/PreviewWeb').PreviewWeb<any>;

Tech: improve global types #20184

Tech: improve global types #20184

Conversation

ndelangen commented Dec 9, 2022 • edited by IanVS Loading

What I did

ndelangen commented Dec 9, 2022

IanVS commented Dec 12, 2022

ndelangen commented Dec 13, 2022

IanVS left a 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

IanVS commented Dec 13, 2022

ndelangen commented Dec 13, 2022

ndelangen commented Dec 9, 2022 •

edited by IanVS

Loading