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

Future - restructure storybook types #19580

Merged
merged 49 commits into from
Oct 25, 2022

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Oct 22, 2022

I'm afraid this isn't going to be easy to review..

The problem:

  • We have many duplicated types in our codebase
  • Users have difficulty getting the types they are lookin for, it's generally confusing.
  • Our types are sometimes incorrect (because of duplication)
  • It's hard to ensure the same interface is used across packages

The solution:

  • Move types scattered in the codebase to a single place
  • Create a new package where all types are exported from

The procedure:

  • Rename types from existing places so they are name-spaced
  • Move the types into the new package 1-by-1
  • Fixing issues at points of usage every time we do the above
  • Remove re-exported types where this happens
  • Removed types exports from packages that had their types moved

This leaves this PR with all sorts of duplicated types that are name-spaced based on where they came from.
But they are all located in the same place/package.

The next obvious step is to truly de-duplicate the types.
But this will open many many instances where types that looked duplicated vary slightly, but significant enough to cause types conflicts.
Resolving those conflicts is going to be painful but necessary.

But who will do that work and when?
I currently do not yet know the answer.

This IS already a breaking change, mainly because of:

  • Removed types exports from packages that had their types moved
  • Rename types from existing places so they are name-spaced

Could I make this backwards compatible? Yes.
Do I think we should? No.

When we start cleaning up, de-duplicate types.. will that result in breaking changes again? Yes.
And this is where I think we might need to balance things out.
I doubt we'll have completed all that work before 7.0 release. So we'll likely have to start deprecating certain types over the course of 7.0's life-time?


I telescoped this PR off of: #19553

@ndelangen ndelangen changed the title Norbert/sb-799-create-a-storybooktypes Future - restructure storybook types Oct 22, 2022
@ndelangen ndelangen self-assigned this Oct 22, 2022
@ndelangen ndelangen added the maintenance User-facing maintenance tasks label Oct 22, 2022
@socket-security
Copy link

socket-security bot commented Oct 22, 2022

Socket Security Report

👍 No new dependency issues detected in pull request

Socket.dev scan summary
Issue Status
Did you mean? ✅ no new possible package typos
Install scripts ✅ no new install scripts
Telemetry ✅ no new telemetry
Troll package ✅ no new troll packages
Malware ✅ no new malware
Native code ✅ no new native modules
Bin Script Confusion ✅ no new bin script confusions
Bin script shell injection ✅ no new bin script shell injection
Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@2.4.2

⚠️ Please accept the latest app permissions to ensure bot commands work properly. Accept the new permissions here.

Powered by socket.dev

@ndelangen ndelangen requested review from shilman and tmeasday October 23, 2022 12:10
@tmeasday
Copy link
Member

@ndelangen

Do you have an idea of how many types there are with duplicate names? Do you have an idea of how many namespaced types we are exporting from the @storybook/types here?

I feel like we are possibly making a lot more work for ourselves to rename all the API_X style types down the track to deal with potentially relatively few duplicates.

Or is your intention that we examine every API_X style type now, even if it isn't duplicated?

@ndelangen
Copy link
Member Author

ndelangen commented Oct 24, 2022

Yes @tmeasday. I'd propose we ultimately want to end up with types without name-spaces.

I tried this PR before without names-acing and got stuck hard on types such as DecoratorFn, Story, StoryIndex, Addon, Ref, Collection, RequireContext, Entry, ArgTypes, Args.

After getting so stuck, and not being able to resolve these conflicts, I diced to take the safe path, and namespace everything, and resolve the actual duplications in a second PR.

I suspect finding some of the name-spaced types that are NOT duplicated will be easy, renaming them to something that makes sense without a namespace will also be easy.

But like I said, I was planning to do those steps in a separate PR. Unless you have a different opinion?

@tmeasday
Copy link
Member

I don't have a strong enough opinion to invalidate the work you already did, so let's go for it. I'd be happy to work through those types above; perhaps create a ticket for me next cycle?

@ndelangen ndelangen merged commit 65957e6 into next Oct 25, 2022
@ndelangen ndelangen deleted the norbert/sb-799-create-a-storybooktypes branch October 25, 2022 16:51
@ndelangen ndelangen restored the norbert/sb-799-create-a-storybooktypes branch November 1, 2022 13:02
@ndelangen ndelangen deleted the norbert/sb-799-create-a-storybooktypes branch November 1, 2022 13:02

export * from './queryparams';

export * from '@storybook/store';
Copy link
Member

Choose a reason for hiding this comment

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

This removes the useArgs hook that is in our public documentation @ndelangen

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding it back:
#19720

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants