Remove getDynamicStyles from core #1000
Conversation
|
|
||
| export type Options = UpdateOptions | ||
|
|
||
| function getObservableStyles(styles: Object): Object | null { |
There was a problem hiding this comment.
we are not using this anywhere, why having it?
There was a problem hiding this comment.
We will need it for proper integration into react-jss, but I will make that in a different PR.
There was a problem hiding this comment.
What integration are you talking about? mb we should do it differently in that case.
There was a problem hiding this comment.
Proper observables integration into react-jss
There was a problem hiding this comment.
But I'm also not sure If we actually need observable support in react-jss.
There was a problem hiding this comment.
If we end up having a function that knows about both, functions and observables, it shouldn't be in one of those plugins, but somewhere else. Duplicating that function in both is also not very nice, it might need some adjustments in the future and having it twice doesn't make it easier to maintain.
There was a problem hiding this comment.
But why do we need to have a function which knows about both?
There was a problem hiding this comment.
I think that these function should live in their respective plugin packages and not in the core.
There was a problem hiding this comment.
I agree that core is not the ideal place for them, but I want to keep performance-first constraint. If we need to loop over styles twice just for the sake of functions living in separate packages, it is not quite performance-first.
There was a problem hiding this comment.
The only place we are using getDynamicStyles at the moment is in react-jss. And without #1002 there will be no performance loose.
|
Btw. another reason to move utilities like this one to a separate package jss-utils is to make every plugin that depends on them treeshakable, since those are pure |
|
They are also tree-shakable right now. Also this PR even contains a small performance boost for react-jss because we only extract the function styles and not the observables. |
|
Nope, since we import from a single entry point and jss package entry point has side effects, nothing that we export from jss package will be treeshakable. |
|
The util functions are now in their respective plugin packages which are pure and don't contain side effects. |
|
Right, forgot about it |
|
Considered latest developments, we should move that function to react-jss package |
|
Closing in favor of this one #1303 |
What would you like to add/fix?
Remove getDynamicStyles from the core and create utility function in plugin repos