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

Move @types/node to dependencies and accept v16 types #16904

Merged

Conversation

Methuselah96
Copy link
Contributor

@Methuselah96 Methuselah96 commented Dec 4, 2021

Issue: N/A

What I did

Error I'm trying to fix:

../../.yarn/__virtual__/@storybook-react-virtual-c572635270/0/cache/@storybook-react-npm-6.4.7-9297a6b4b2-a834b3042a.zip/node_modules/@storybook/react/dist/ts3.9/client/preview/index.d.ts(2,23): error TS2688: Cannot find type definition file for 'node'.

This happens because app/react/src/client/preview/index.d.ts ends up referencing @types/node when compiled:

/// <reference types="webpack-env" />
/// <reference types="node" />
import { ClientStoryApi, Loadable } from '@storybook/addons';
import './globals';
import { IStorybookSection } from './types';
import { ReactFramework } from './types-6-0';
interface ClientApi extends ClientStoryApi<ReactFramework['storyResult']> {
    setAddon(addon: any): void;
    configure(loader: Loadable, module: NodeModule): void;
    getStorybook(): IStorybookSection[];
    clearDecorators(): void;
    forceReRender(): void;
    raw: () => any;
}
export declare const storiesOf: ClientApi['storiesOf'];
export declare const configure: ClientApi['configure'];
export declare const addDecorator: ClientApi['addDecorator'];
export declare type DecoratorFn = Parameters<typeof addDecorator>[0];
export declare const addParameters: ClientApi['addParameters'];
export declare const clearDecorators: ClientApi['clearDecorators'];
export declare const setAddon: ClientApi['setAddon'];
export declare const forceReRender: ClientApi['forceReRender'];
export declare const getStorybook: ClientApi['getStorybook'];
export declare const raw: ClientApi['raw'];
export {};

Since other dependent packages already depend on @types/node, this shouldn't cause any problems.

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Dec 4, 2021

Nx Cloud Report

CI ran the following commands for commit e580e39. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@@ -57,6 +57,7 @@
"@storybook/react-docgen-typescript-plugin": "1.0.2-canary.6.9d540b91e815f8fc2f8829189deb00553559ff63.0",
"@storybook/semver": "^7.3.2",
"@storybook/store": "6.5.0-alpha.1",
"@types/node": "^14.14.20",

Choose a reason for hiding this comment

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

^14.14.20 || ^16.0.0? And move @types/node to dependencies for other packages too?

See https://github.com/storybookjs/storybook/pull/16894/files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added it for the other relevant packages. I'll keep it as 14 unless we somehow combine these PRs.

@Methuselah96 Methuselah96 changed the title Move @types/node to dependencies for @storybook/react Move @types/node to dependencies Dec 5, 2021
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Hi @Methuselah96 thanks so much for this contribution! 🙏

I did a quick spot check and it looks like preact, html, and bunch of other app/* follows the same pattern. Would you mind:

Would love to merge this ASAP!

@shilman shilman added the linear label Dec 6, 2021
@Methuselah96
Copy link
Contributor Author

@shilman Good catch! Updated!

@Methuselah96 Methuselah96 force-pushed the move-types-node-to-dependencies branch from 0ff0aa7 to adaec7d Compare December 7, 2021 00:27
@Methuselah96 Methuselah96 changed the title Move @types/node to dependencies Move @types/node to dependencies and accept v16 types Dec 7, 2021
@Methuselah96
Copy link
Contributor Author

@shilman I think I've finally got the build passing. It took more changes than I anticipated. Not sure why "chromatic" is failing with "Failed to verify your Storybook", but it seems unrelated to this PR. Let me know if there's anything else I can do.

@Methuselah96
Copy link
Contributor Author

Looks like a rebuild fixed "chromatic."

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Thanks @Methuselah96 for updating this, and also apologies for the struggles getting CI to pass -- I wish it was faster & easier. At any rate, really appreciate it!

@shilman shilman merged commit 99769f8 into storybookjs:next Dec 7, 2021
@Methuselah96 Methuselah96 deleted the move-types-node-to-dependencies branch December 7, 2021 16:02
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.

4 participants