search

cssinjs / jss

JSS is an authoring tool for CSS which uses JavaScript as a host language.
https://cssinjs.org
MIT License
7.07k stars 397 forks source link

getDynamicStyles() includes Observable rules #930

Open bhupinderbola opened 5 years ago

bhupinderbola commented 5 years ago

Expected behavior: Observable rules should not be included in the getDynamicStyles()

Describe the bug: Observable rules are included in the results of getDynamicStyles()

Codesandbox link: https://codesandbox.io/s/xrmo482nr4

Versions (please complete the following information):

HenriBeck commented 5 years ago

Can you please explain why this is unexpected? Observables are a form of dynamic styles.

bhupinderbola commented 5 years ago

As per the documentation (https://cssinjs.org/js-api?v=v9.8.7#extract-dynamic-styles ),

Extracts a styles object with only props that contain function values

. Also it creates issue with react-jss. React-jss calls sheet.update on dynamic styles. But observables do not have any data to update.

kof commented 5 years ago

Or we should fix the problem in react-jss by adding a check for fn values

HenriBeck commented 5 years ago

Yeah, we should fix the docs and fix react-jss by adding a check.

Is there a use case for observables inside react-jss currently?

kof commented 5 years ago

Same use case as with core, being able to stream the values from thee outside, for e.g. a complex animation using rx utils

HenriBeck commented 5 years ago

But this would break as soon as you render two different instances of the same component. You would need to create a new HOC for every rendered component.

kof commented 5 years ago

Why would it break? We don't reuse dynamic style between elements.

HenriBeck commented 5 years ago

Yeah, but the observable would be the same. You can only have an animation which is shared between all components

HenriBeck commented 5 years ago

ToDos for this:

kof commented 5 years ago

True that. I wonder if we should support a fn value that returns an observable.

kof commented 5 years ago

Alternatively separate components per animation might be also acceptable.

HenriBeck commented 5 years ago

Please no. This would just create more confusion. We would also need a way to pass the observable then back to the user.

We should definitely document the problem with observables and react-jss somewhere.

kof commented 5 years ago

yeah, the user would have to manage observable instances.

kof commented 5 years ago

Probably for now user could just create separate components for each animation and scope the observable in his HOC creator function.

HenriBeck commented 5 years ago

I think for most users function values should be enough.

kof commented 5 years ago

Totally, but then we have this Observables feature and it would be evtl nice to be able to use it with react-jss together. I don't know how to decide on whether it should be supported or not, I think if its not hard and does't causes additional bugs, we could. That thing per component can be solved in user space ... I guess your concern comes from implicitness of this behaviour one wouldn't expect?

HenriBeck commented 5 years ago

Yeah, exactly.

kof commented 5 years ago

On the other side it would be confusing not having support for observables in react-jss because it claims to support the same syntax, and Observables are part of it.