Skip to content

Future - restructure storybook types#19580

Merged
ndelangen merged 49 commits into
nextfrom
norbert/sb-799-create-a-storybooktypes
Oct 25, 2022
Merged

Future - restructure storybook types#19580
ndelangen merged 49 commits into
nextfrom
norbert/sb-799-create-a-storybooktypes

Conversation

@ndelangen

@ndelangen ndelangen commented Oct 22, 2022

Copy link
Copy Markdown
Member

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

socket-security Bot commented Oct 22, 2022

Copy link
Copy Markdown

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
Copy Markdown
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

ndelangen commented Oct 24, 2022

Copy link
Copy Markdown
Member Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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