added properties as functions to free-style #47
Conversation
| @@ -170,10 +170,11 @@ function stringifyProperties (properties: Properties) { | |||
|
|
|||
| for (const [name, value] of properties) { | |||
| if (value != null) { | |||
| if (Array.isArray(value)) { | |||
| result.push(value.filter(x => x != null).map(x => styleToString(name, x)).join(';')) | |||
| const value2 = typeof value === 'function' ? value() : value | |||
There was a problem hiding this comment.
I'll tidy it up, but initial issue is that value2 may be null. Should probably be const value = typeof prop === 'function' ? prop() : prop before checking the value.
There was a problem hiding this comment.
The other issue with using functions is that the code is no longer serialisable and there's a minor fear that this will introduce incompatibilities with CSS-in-JS implementation since it can not be statically analysed anymore. For instance, it's possible to use PostCSS with free-style, but this breaks that. It also break other static analysis tooling and infrastructure - vendor prefixing, etc. will all be broken. I'm going to hold off merging this as a result, sorry!
There was a problem hiding this comment.
Can you help me understand what would be broken and how? The expectation with this function is that it is only ever called once.
There was a problem hiding this comment.
PostCSS, vendor prefix, things that expect numbers or strings. All modules that can currently augment a style object would need to be notified that this change in expectations has occurred, as it won't be very useful for a style processor to skip a function instance because it doesn't understand it (or worse, crash or omit it altogether thinking it's invalid) and, as a result, not update the style with vendor prefixes.
There was a problem hiding this comment.
I will test that out in postcss and autoprefixer and see how they react. In a perfect world they should simply copy the value of each property onto each prefixed property.
There was a problem hiding this comment.
And what about vendor prefixing of the value and not the property name? It would be skipped in that case. I don't think it's a worthwhile change to break expectations at this point, that's all.
There was a problem hiding this comment.
Alright, well it was worth a try. Will close this PR for now.
There was a problem hiding this comment.
No worries. I thought it was a nice solution too, I'd just rather maintain compatibility with the larger ecosystem. There's also possibilities in the future where we could inline all of free-style, but function calls make that a little trickier.
1 similar comment
1 similar comment
| // check again for null because value could be null | ||
| if (value2 == null) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
There's really no need for both these checks. Just moving the one check would have been adequate (two lines changed) as typeof null === 'object'.
Added the ability for properties to be provided as functions. This should allow free-style to support helper libraries or properties that can be calculated at the last moment.