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

Remove @storybook/addon-storyshots types #41583

Merged
merged 2 commits into from Jan 15, 2020
Merged

Remove @storybook/addon-storyshots types #41583

merged 2 commits into from Jan 15, 2020

Conversation

gaetanmaisse
Copy link
Contributor

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.

Select one of these and delete the others:

If removing a declaration:

  • Delete the package's directory.
  • Add it to notNeededPackages.json.

--

@storybook/addon-storyshots and @storybook/addon-storyshots-puppeteer were migrated to TS in this PR storybookjs/storybook#4758 and publically available with Storybook 5.3.0 release.

…by the package directly

TS migrate of the package was done in storybookjs/storybook#7674
… provided by the package directly

TS migrate of the package was done in storybookjs/storybook#7674
@typescript-bot
Copy link
Contributor

typescript-bot commented Jan 14, 2020

@gaetanmaisse Thank you for submitting this PR!

🔔 @bradleyayers @Yama-Tomo - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@typescript-bot typescript-bot moved this from Waiting for Reviewers to Check and Merge in Pull Request Status Board Jan 14, 2020
@typescript-bot typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. Merge:Express and removed Awaiting reviewer feedback labels Jan 14, 2020
@typescript-bot
Copy link
Contributor

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

Copy link
Contributor

@Yama-Tomo Yama-Tomo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@elibarzilay elibarzilay merged commit dfbf2d1 into DefinitelyTyped:master Jan 15, 2020
Pull Request Status Board automation moved this from Check and Merge to Done Jan 15, 2020
@gaetanmaisse gaetanmaisse deleted the remove-storybook-addon-storyshots-types branch January 15, 2020 09:13
@SimenB
Copy link
Contributor

SimenB commented Jan 15, 2020

we get this now - is it caused by the change in this pr?

error Couldn't find package "storybook__addon-storyshots@*" required by "@types/storybook__addon-storyshots@^5.0.0" on the "npm" registry.

@gaetanmaisse
Copy link
Contributor Author

gaetanmaisse commented Jan 15, 2020

@SimenB it is surely related. As @storybook/addon-storyshots now exports its own types I removed the old and not up to date ones from DefinitelyTyped. You can either use the last version available on DefinitelyTyped i.e. @types/storybook__addon-storyshots@5.1.2 or remove it completely and use types provided by the package itself.
Didn't expect to throw an error like this and not fall back to the latest available types 😞

@SimenB
Copy link
Contributor

SimenB commented Jan 15, 2020

Installing it shouldn't error though, that's a bug

@gaetanmaisse
Copy link
Contributor Author

Great, I'm not crazy, that's not the correct behavior.
Not great, it looks like there is something broken for all scoped packages.

@SimenB
Copy link
Contributor

SimenB commented Jan 16, 2020

@sandersn seems to be a bug in the publisher?

@sandersn
Copy link
Contributor

Yep, looks like the publisher forgets to unmangle the name before adding the dependency.

The dependency is kind of bogus anyway -- it's a deprecated package!

@sandersn
Copy link
Contributor

It should now be fixed for future deprecations; I didn't fix the 38 existing packages that the buggy code generated because (1) it's a royal pain (2) the workaround -- deleting the @types/storybook__addon-storyshots dependency -- seems easy. I'm happy to discuss whether I'm wrong about that.

@sandersn
Copy link
Contributor

Never mind, I decided to fix them. The fixup script wasn't that hard to write...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Owner Approved A listed owner of this package signed off on the pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants