Dynamic values full syntax #878

kof · 2018-10-09T02:04:44Z

kof · 2018-10-09T02:07:56Z

packages/jss-plugin-syntax-rule-value-function/src/index.js

+        // We need to run the plugins in case style relies on syntax plugins.
+        const process = options ? options.process : true
+        if (process) {
+          styleRule.options.jss.plugins.onProcessStyle(styleRule.style, styleRule, sheet)


don't really like this here, that api was supposed to be private, but I haven't found a way around so far.

Can't this be moved into the core and then diff it against the previous style object and only update the properties which are needed?

Also I thought we would go with the route of fastValue((props) => props.color) to make fast function values?

strict comparison of previous and current style reference before the hook and after might actually be a good idea! Gonna try this!

fastValue we can still add as a separate feature later on, I think we can skip this for now, just because we still have observables for "fast by default" and its not clear if anyone actually using function values as "fast by default" with frequent updates besides of my demos.

also sheet.update(data, {process: false}) is an equevalent, so we might not need fastValue() at all

We could also make a shallow comparison right?

Also what do you think of only supporting fn values as fast and not fn rules?

Regular updates with process: true is not literally slow, its just with more overhead, it is still very fast. I think the only use case that might want that is animations. In that case, it shouldn't be hard to set that option, also I still don't know if anyone using it that way, so I would just wait to see if we learn more about such cases.

We could also make a shallow comparison right?

Yeah, can be done if needed, I think we only need to check style references currently.

Also what do you think of only supporting fn values as fast and not fn rules?

The problem is that values with jss-expand can be complex too, for e.g. margin: {top, left}, so you will have a painful inconsistency if you make them fast by default.

No, not make them fast by default but instead only allowing 'fast' functions as function values.

same thing, no? margin: () => ({top, left})

kof · 2018-10-09T02:09:34Z

packages/jss/src/RuleList.js

@@ -127,16 +138,36 @@ export default class RuleList {
  /**
   * Update the function values with a new data.
   */
-  update: Update = (name, data) => {
+  update(...args: UpdateArguments): void {


previous implementation was generating arrow method, which transpiles to a less performant inlined function in constructor

- move documentation to the package - update docs, since plugins don't apply by default - introduce option process: true to the plugin to enable plugins if user wants that

kof · 2018-10-11T16:12:44Z

Ok I am done, please take a look again

kof · 2018-10-11T17:52:09Z

Lets review and merge this one for now, there are 2 issues we need to think how to solve separately.

…d values

kof · 2018-10-14T18:16:12Z

changelog.md

+
+- Observables, function values and rules are now standalone packages, not part of the core. They are still part of the default preset though.
+- Function values and rules apply plugins by default now, which means they are slower by default. To speed them up use `sheet.update(data, {process: false})` (#682)
+- Observables will not apply plugins by default now, to make them even faster. If you want them to use plugins, can use an option when using the plugin: `jss.use(pluginObservable({process: true}))` (#682)


#746 (comment)

HenriBeck · 2018-10-14T18:22:02Z

packages/jss/src/RuleList.js

+    const styleRule: StyleRule = (rule: any)
+    const {style} = styleRule
+
+    plugins.onUpdate(data, rule, sheet, options)


Could we maybe return the new style from the update hook? Seems to me a little bit cleaner.

dunno, onUpdate doesn't mean onUpdateStyles, it means user called .update(), so not sure what would be a good return there, thats why I didn't return anything

HenriBeck · 2018-10-14T18:22:52Z

packages/jss/src/RuleList.js

+      // We need to run the plugins in case new `style` relies on syntax plugins.
+      plugins.onProcessStyle(styleRule.style, styleRule, sheet)
+      // Update, remove and add props.
+      let nextValue


Why do you initialize them here? can't we just use const inside the for loop?

we do, I was thinking we should avoid creating more variables in hot places to reduce garbage collection, but on the other hand, transpiled variable will be hoisted anyways, so there is no difference and we can do it in a more readable way

onProcessStyle is invoked on each .update, but in this case we only need to do this once, because fn values can't be added

kof · 2018-10-14T21:42:40Z

🎉🎉🎉

* master: Support plugins processing for observables by default (#892) Typed CSSOM values support (#887) Dynamic values full syntax (#878) [jss-plugin-cache] Use WeakMap for cache plugin (#890) # Conflicts: # packages/jss-plugin-syntax-rule-value-function/.size-snapshot.json # packages/jss-preset-default/.size-snapshot.json # packages/jss/.size-snapshot.json # packages/react-jss/.size-snapshot.json

* origin/add-ts-typings: Scoped keyframes (#888) Update readme.md (#897) Use CSSTOM in default unit plugin (#893) Added .nvmrc (#896) [WIP] DONT MERGE Fix ci (#895) Update ci (#894) Support plugins processing for observables by default (#892) Typed CSSOM values support (#887) Dynamic values full syntax (#878) [jss-plugin-cache] Use WeakMap for cache plugin (#890)

* added fallbacks test to fn rules * only call onChangeValue if plugins: true * enable removing props from fn rules * shorter syntax with coercion * wip full syntax support * restore browsers.json * changelog * move hook call to the core * isProcessed flag explanation * added tests to fn rules * remove media rule as a function test case * added a test for cssinjs#796, as part of cssinjs#682 * tests for compose plugin * observables - move documentation to the package - update docs, since plugins don't apply by default - introduce option process: true to the plugin to enable plugins if user wants that * move fn values docs * wording * Added tests for using jss-plugin-syntax-expand with function rules and values * Skip fn values with jss-expand for now * move container rules update to the core * optimize onProcessStyle onProcessStyle is invoked on each .update, but in this case we only need to do this once, because fn values can't be added * put variables in the right scope

kof added 7 commits September 18, 2018 12:01

added fallbacks test to fn rules

b90fe69

only call onChangeValue if plugins: true

2989e27

enable removing props from fn rules

dfbb767

shorter syntax with coercion

2296958

Merge branch 'master' into dynamic-values-full-syntax

bdd1ddf

wip full syntax support

421d42b

Merge branch 'master' into dynamic-values-full-syntax

901ae2f

kof requested review from TrySound and HenriBeck October 9, 2018 02:04

restore browsers.json

79228d2

kof changed the title ~~Dynamic values full syntax~~ [WIP] Dynamic values full syntax Oct 9, 2018

kof commented Oct 9, 2018

View reviewed changes

kof added 9 commits October 9, 2018 10:50

changelog

3583f32

move hook call to the core

9a6a1e0

isProcessed flag explanation

49cc07f

added tests to fn rules

7e5eca9

remove media rule as a function test case

abfafa8

added a test for #796, as part of #682

c5fdf05

tests for compose plugin

a70b3e2

observables

54f2b7f

- move documentation to the package - update docs, since plugins don't apply by default - introduce option process: true to the plugin to enable plugins if user wants that

move fn values docs

220bb42

kof changed the title ~~[WIP] Dynamic values full syntax~~ Dynamic values full syntax Oct 11, 2018

wording

d5b2794

kof mentioned this pull request Oct 12, 2018

Typed CSSOM values support #887

Merged

Henri Beck added 2 commits October 13, 2018 16:17

Added tests for using jss-plugin-syntax-expand with function rules an…

171902b

…d values

Skip fn values with jss-expand for now

0e53702

kof commented Oct 14, 2018

View reviewed changes

HenriBeck reviewed Oct 14, 2018

View reviewed changes

kof added 3 commits October 14, 2018 21:20

move container rules update to the core

efede57

optimize onProcessStyle

59f5ccc

onProcessStyle is invoked on each .update, but in this case we only need to do this once, because fn values can't be added

put variables in the right scope

b17c63d

kof merged commit 92a53e4 into master Oct 14, 2018

HenriBeck mentioned this pull request Oct 22, 2018

Support full syntax in function rules and values #682

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic values full syntax #878

Dynamic values full syntax #878

kof commented Oct 9, 2018 •

edited

Loading

kof Oct 9, 2018

HenriBeck Oct 10, 2018

HenriBeck Oct 10, 2018

kof Oct 10, 2018 •

edited

Loading

kof Oct 10, 2018 •

edited

Loading

HenriBeck Oct 10, 2018

kof Oct 10, 2018

kof Oct 10, 2018

HenriBeck Oct 10, 2018 •

edited

Loading

kof Oct 10, 2018 •

edited

Loading

kof Oct 9, 2018

HenriBeck Oct 10, 2018

kof commented Oct 11, 2018

kof commented Oct 11, 2018

kof Oct 14, 2018

HenriBeck Oct 14, 2018

kof Oct 14, 2018

HenriBeck Oct 14, 2018

kof Oct 14, 2018

kof commented Oct 14, 2018

Dynamic values full syntax #878

Dynamic values full syntax #878

Conversation

kof commented Oct 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kof Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

kof Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HenriBeck Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

kof Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kof commented Oct 11, 2018

kof commented Oct 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kof commented Oct 14, 2018

kof commented Oct 9, 2018 •

edited

Loading

kof Oct 10, 2018 •

edited

Loading

kof Oct 10, 2018 •

edited

Loading

HenriBeck Oct 10, 2018 •

edited

Loading

kof Oct 10, 2018 •

edited

Loading