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

Fix Event after changes to @types/node #19373

Closed
wants to merge 2 commits into from
Closed

Conversation

thw0rted
Copy link

@thw0rted thw0rted commented Oct 6, 2022

After merging my recent PR for @types/node, I was alerted that a couple dependents of this package were failing to compile. I have identified a fix that I believe should preserve the original behavior of this code while resolving those errors. I don't know anything about Storybook, so if I'm wrong, or if there's a better fix for the issue, please feel free to do your own thing.

For background: I don't think Storybook, or the packages with failing dtslint runs, storybook-readme and storybook-addon-jsx, directly depend on @types/node, but it's common for frontend packages to install build tooling that does depend on the package. This causes Node types to be introduced to the default typeRoot, so you wind up with Node types merged in with those in the DOM lib. (Storybook may be a bit of an odd duck, because it looks like in some cases, React declares its own empty DOM interfaces in @types/react/globals.d.ts.)

At any rate, I hope the proposed fix works for you.

After merging my [recent PR](DefinitelyTyped/DefinitelyTyped#59905) for `@types/node`, I was alerted that a couple dependents of this package were failing to compile.  I have identified a fix that I believe should preserve the original behavior of this code while resolving those errors.  I don't know anything about Storybook, so if I'm wrong, or if there's a better fix for the issue, please feel free to do your own thing.

For background: I don't think Storybook, or the packages with failing `dtslint` runs, `storybook-readme` and `storybook-addon-jsx`, directly depend on `@types/node`, but it's common for frontend packages to install build tooling that does depend on the package.  This causes Node types to be introduced to the default `typeRoot`, so you wind up with Node types merged in with those in the DOM lib.  (Storybook may be a bit of an odd duck, because it looks like in some cases, React declares its own empty DOM interfaces in `@types/react/globals.d.ts`.)

At any rate, I hope the proposed fix works for you.
sandersn added a commit to microsoft/DefinitelyTyped-tools that referenced this pull request Oct 7, 2022
storybook types copy+extend a Node type (EventTarget) and rely on structural typing to make
them assignable. But [this fails with the newest version of node
types](https://dev.azure.com/definitelytyped/DefinitelyTyped/_build/results?buildId=141198&view=logs&j=9d906afe-3eb5-5d79-316d-86f3f3fa360c&t=09716206-abc7-5ba2-2ea2-6c6fe97a3aad).
@thw0rted has a PR storybookjs/storybook#19373
to make storybook correctly extend EventTarget, but I'm going to disable
the failures for these four packages until the new version of storybook
ships.
sandersn added a commit to microsoft/DefinitelyTyped-tools that referenced this pull request Oct 7, 2022
storybook types copy+extend a Node type (EventTarget) and rely on structural typing to make
them assignable. But [this fails with the newest version of node
types](https://dev.azure.com/definitelytyped/DefinitelyTyped/_build/results?buildId=141198&view=logs&j=9d906afe-3eb5-5d79-316d-86f3f3fa360c&t=09716206-abc7-5ba2-2ea2-6c6fe97a3aad).
@thw0rted has a PR storybookjs/storybook#19373
to make storybook correctly extend EventTarget, but I'm going to disable
the failures for these four packages until the new version of storybook
ships.
@kasperpeulen
Copy link
Contributor

It seems that @tmeasday already fixed this in:

5f46e8c

But thanks for looking into this!

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.

None yet

3 participants