-
-
Notifications
You must be signed in to change notification settings - Fork 9.5k
FIX singleton module issue for manager & theme not being set correctly #5679
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
Conversation
if no proper theme is provided, we use light / shallow-merge in light base
lib/theming/src/create.ts
Outdated
@@ -177,3 +178,35 @@ export const create = (vars: ThemeVar, rest?: Rest): Theme => ({ | |||
|
|||
...(rest || {}), | |||
}); | |||
|
|||
export const ensure = (input: any): Theme => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this safety check. I feel like this is going to hide bugs. Can we fail fast here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fail when anything is missing in the theme?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, so the stack trace leads you to the error check, rather than in the depths of some component where it's unclear whether it's a bug in the theme or a bug in the component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree on your point that it may hide bugs, but I think throwing on a missing property will cause issues later if we're adding something to the theme.
In that scenario, anything we add to the theme object, will break every theme users have made.
I think some grace is appropriate here, likely the worst that is to happen is, some styling inconsistency in someone's storybook, not broken one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, this looks better to me. Thanks!
Codecov Report
@@ Coverage Diff @@
## next #5679 +/- ##
==========================================
- Coverage 33.35% 33.24% -0.12%
==========================================
Files 647 648 +1
Lines 9332 9363 +31
Branches 1324 1330 +6
==========================================
Hits 3113 3113
- Misses 5604 5635 +31
Partials 615 615
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this, the resolve solution for singletons is about the best we can do.
Also warning on missing theme props is probably what we want.
👍
FIX singleton module issue for manager & theme not being set correctly
Issue: #5663
What I did