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

[@storybook__*] Delete Storybook types #38447

Merged

Conversation

@MichaelDeBoey
Copy link
Contributor

MichaelDeBoey commented Sep 17, 2019

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.

All packages were migrated to TS (which is tracked in storybookjs/storybook#5030) and provide their own typings.

I deleted the typings for the following packages:

  • @storybook/addon-actions
  • @storybook/addon-backgrounds
  • @storybook/addon-centered
  • @storybook/addon-jest
  • @storybook/addon-knobs
  • @storybook/addon-links
  • @storybook/addon-options
  • @storybook/addon-viewport
  • @storybook/addons
  • @storybook/channels
  • @storybook/html
  • @storybook/preact
    Set to 5.2.1 since 5.2 typings were already published here even tho that package version wasn't released yet
  • @storybook/react
  • @storybook/react-native
  • @storybook/vue
@typescript-bot

This comment has been minimized.

Copy link
Contributor

typescript-bot commented Sep 17, 2019

@MichaelDeBoey Thank you for submitting this PR!

🔔 @joscha @jicjjang @HyunSeob @adhrinae @kiyopikko @halfmatthalfcat @martynaskadisa @amacleay @MLoughry @alanhchoi @azmenak @jessepinho @simonhn @gaetanmaisse @adam187 @Vinnl @bmatcuk @ChristianMurphy @9renpoto @ddayguerrero @wapgear @alechill @iRoachie @ceyhuno @pntgupta @jurgisrudaks @yoyoys - 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 Needs Author Attention in Pull Request Status Board Sep 18, 2019
@typescript-bot

This comment has been minimized.

Copy link
Contributor

typescript-bot commented Sep 18, 2019

@MichaelDeBoey The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Copy link
Contributor

joscha left a comment

What happens to consumers of pre-5.2.0 versions if there are any problems with the types? Adding the packages to notNeededPackages.json looks good to me, but I think we should keep the typings around for a little longer?

Copy link
Contributor

joscha left a comment

Requesting changes until the question is addressed, slo it doesn't accidentally get merged.

Copy link
Contributor

joscha left a comment

.

@MichaelDeBoey

This comment has been minimized.

Copy link
Contributor Author

MichaelDeBoey commented Sep 18, 2019

@joscha I just followed the workflow as explained in the PR template and the docs.

Other deleted types (see #34111 & #37358) just deleted the corresponding folders too.

@joscha

This comment has been minimized.

Copy link
Contributor

joscha commented Sep 18, 2019

@joscha I just followed the workflow as explained in the PR template and the docs .

ah, so the output of this PR is from npm run not-needed -- typingsPackageName asOfVersion sourceRepoURL [libraryName].? I guess that's expected then? Feels a bit strange that we wouldn't be able to update typings for older versions any more, but 🤷‍♂

@MichaelDeBoey

This comment has been minimized.

Copy link
Contributor Author

MichaelDeBoey commented Sep 18, 2019

@joscha Yes it does 🙂

@MichaelDeBoey

This comment has been minimized.

Copy link
Contributor Author

MichaelDeBoey commented Sep 30, 2019

@MichaelDeBoey

This comment has been minimized.

Copy link
Contributor Author

MichaelDeBoey commented Sep 30, 2019

I've reverted the removal of the @storybook/react, so this can already be merged.

I'll create a new PR for it when this one's merged. 🙂

@ndelangen

This comment has been minimized.

Copy link
Contributor

ndelangen commented Sep 30, 2019

What can I do to help here?

@MichaelDeBoey

This comment has been minimized.

Copy link
Contributor Author

MichaelDeBoey commented Sep 30, 2019

@ndelangen I'll tag you in the new PR after this one's merged.
But that one will have the same issue as we had here, so maybe you can have a look at the error that we had? 🤔

https://travis-ci.org/DefinitelyTyped/DefinitelyTyped/builds/591252348

@typescript-bot

This comment has been minimized.

Copy link
Contributor

typescript-bot commented Sep 30, 2019

🔔 @joscha - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate?

@MichaelDeBoey

This comment has been minimized.

Copy link
Contributor Author

MichaelDeBoey commented Oct 4, 2019

Ping @joscha @ExE-Boss for approval

@joscha

This comment has been minimized.

Copy link
Contributor

joscha commented Oct 4, 2019

Sorry, I still don't really have an answer as to what happens with the pre-5.2.0 versions after we merge this, so I don't really know whether this should be approved or not.

@MichaelDeBoey

This comment has been minimized.

Copy link
Contributor Author

MichaelDeBoey commented Oct 4, 2019

@joscha This is just the normal flow of deleting typings from the repo.

Since their's a new version published to npm when we update typings, I guess this won't happen anymore and the old @types/storybook__* will still be existing on the npm registry.

@joscha

This comment has been minimized.

Copy link
Contributor

joscha commented Oct 4, 2019

@MichaelDeBoey

This comment has been minimized.

Copy link
Contributor Author

MichaelDeBoey commented Oct 4, 2019

@joscha I think this is a general thing that needs to be discussed elsewhere, no? 🤔

Maybe temporarily add them back, fix the typing error, publish the fix and delete them again? 🤔
This seems to me like we shouldn't hold back this PR.

@ndelangen

This comment has been minimized.

Copy link
Contributor

ndelangen commented Oct 7, 2019

@joscha We don't actively maintain or support 4.x anymore.

I'm personally not too concerned with this "what happens if" scenario, users have options:

My recommendation would be to use a fork, a local type definition or https://www.npmjs.com/package/patch-package

And hopefully the option of upgrading is available..

My main concern is that storybook 5.x users should remove the packages as soon as possible, because they are incompatible with the 5.x releases.

@joscha

This comment has been minimized.

Copy link
Contributor

joscha commented Oct 7, 2019

@joscha
joscha approved these changes Oct 7, 2019
@typescript-bot typescript-bot moved this from Needs Author Attention to Check and Merge in Pull Request Status Board Oct 7, 2019
@typescript-bot

This comment has been minimized.

Copy link
Contributor

typescript-bot commented Oct 7, 2019

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!

@armanio123 armanio123 merged commit ab4aed4 into DefinitelyTyped:master Oct 7, 2019
3 checks passed
3 checks passed
DefinitelyTyped.BenchmarkPR Build #16838 succeeded
Details
DefinitelyTyped.DefinitelyTyped Build #20190930.19 succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Pull Request Status Board automation moved this from Check and Merge to Done Oct 7, 2019
@MichaelDeBoey MichaelDeBoey deleted the MichaelDeBoey:delete-storybook-types branch Oct 8, 2019
@MichaelDeBoey MichaelDeBoey mentioned this pull request Oct 8, 2019
7 of 7 tasks complete
@Mussabaheen

This comment has been minimized.

Copy link

Mussabaheen commented Oct 11, 2019

GREAT

This was referenced Oct 23, 2019
@MichaelDeBoey MichaelDeBoey mentioned this pull request Nov 18, 2019
7 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.