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

final few deprecations removal in a batch #19553

Merged
merged 17 commits into from
Oct 25, 2022
Merged

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Oct 20, 2022

removal of the last few deprecations for 7.0

@ndelangen ndelangen requested a review from shilman October 20, 2022 08:01
@ndelangen ndelangen self-assigned this Oct 20, 2022
@ndelangen ndelangen added the maintenance User-facing maintenance tasks label Oct 20, 2022
@ndelangen
Copy link
Member Author

@shilman Do you think you can help me understand why https://app.circleci.com/pipelines/github/storybookjs/storybook/30614/workflows/4a03924c-4716-4d9e-ac10-c499ba5fdae8/jobs/429471 is failing? Seems the background color isn't applied anymore, but I can't seem to find the reason why.

@ndelangen
Copy link
Member Author

@tmeasday you marked adding unknown globals as deprecated, but it looks like removing that actually breaks addon-backgrounds?

@tmeasday
Copy link
Member

@tmeasday you marked adding unknown globals as deprecated, but it looks like removing that actually breaks addon-backgrounds?

Well yeah, I guess backgrounds should export a globalTypes definition for BACKGROUNDS_PARAM_KEY ('backgrounds')

@ndelangen
Copy link
Member Author

That fixed it @tmeasday ♥️

@ndelangen
Copy link
Member Author

@shilman mentioned that he wanted to discuss this deprecation further.

He said that silently ignoring new incoming properties was not a good idea. I tend to agree.
We should either fail hard or continue to warn.

@ndelangen
Copy link
Member Author

I'll add a bit of code that throws an error when a property is filtered out

@shilman
Copy link
Member

shilman commented Oct 24, 2022

@tmeasday can you chime in on why we want to force addons to declare their globalTypes before using them? it makes sense intuitively, but is there some specific use case or problem that you have in mind? Just curious to help me understand the change better. Thanks!

@tmeasday
Copy link
Member

tmeasday commented Oct 25, 2022

@shilman, according to the original commit:

This ensures weird stuff doesn't happen when we change arg/global names.

I don't really have any recollection of it, to be honest. I don't feel strongly about it.

Copy link
Member

shilman commented Oct 25, 2022

@tmeasday thanks, just checking. i think we should do it FWIW.

@ndelangen ndelangen merged commit ec0f15f into next Oct 25, 2022
@ndelangen ndelangen deleted the deprecate/remaining-lib-cli branch October 25, 2022 14:30
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