Emotion Primitives #658
Emotion Primitives #658
Conversation
| render(<Title>Hello</Title>) | ||
| */ | ||
| export const emotionPrimitive = tag => { | ||
| if (Platform.OS === 'web') { |
There was a problem hiding this comment.
imho this can be easily split at build time - we can create 2 separate bundles and point from package.json with main/module and react-native keys appropriately
I can help with setting this up later
There was a problem hiding this comment.
That would be great.
| } | ||
|
|
||
| export function splitProps(rootEl, props) { | ||
| const rest = { ...props } |
There was a problem hiding this comment.
this will make a copy of props, seems redundant as u only read from it anyway
There was a problem hiding this comment.
Haha ya I think I forgot to refactor before pushing the code. We can remove this since 'props' is pass by value.
| export function getPrimitiveProps(element, props) { | ||
| const acc = {} | ||
|
|
||
| Object.keys(props).forEach(prop => { |
There was a problem hiding this comment.
iteration style seems inconsistent with splitProps where for similar job you use .reduce
There was a problem hiding this comment.
This will change in next commit. I was testing some weird behaviour caused due to primitive props like onLongPress.
|
@nitin42 I believe you can use this as an example to create the styles. https://github.com/emotion-js/next/blob/master/packages/serialize/src/index.js#L66 |
|
Thanks @tkh44 ! Got it working. Wrapping up the work now by preparing docs, examples and demos for all the platforms (native, web and sketch). |
| @@ -0,0 +1,58 @@ | |||
| import React from 'react' | |||
There was a problem hiding this comment.
import * as React from 'react' works better with typings
There was a problem hiding this comment.
I'll take care of adding flow types in next few commits!
| @@ -0,0 +1,58 @@ | |||
| import React from 'react' | |||
| import transform from 'css-to-react-native' | |||
| import cssToObjects from 'css-to-object' | |||
There was a problem hiding this comment.
probably should be cssToObject
There was a problem hiding this comment.
Not using this anymore! Haha It's better if you don't review this now 😅
There was a problem hiding this comment.
Many things are going to change 😅
| // Resolve the style using StyleSheet.create | ||
| rnStyles = resolveStyles([transform(cssToRN)]) | ||
| } else { | ||
| rnStyles = [...styles] |
There was a problem hiding this comment.
i think spread in general has some extra work to do (like dealing with array holes, using iteration protocol etc), I guess this is known to be an array so styles.concat() should be slightly more performant
There was a problem hiding this comment.
Yup! We can definitely make changes and add bits related to performance. Since that is emotion's trademark 😉
|
Thanks @Andarist for the initial review. Now I know the contributing parts a little better. Stay tuned for some surprise. This requires some more work but it should be stabled by next week. |
| css = cssToObjects(styles.join('')) | ||
|
|
||
| Object.keys(css).forEach(prop => { | ||
| cssToRN.push([prop, css[prop]]) |
There was a problem hiding this comment.
cssToRN is only manipulated here and passed as argument to a function just few lines below, as it's size is known (Object.keys(css).length) it might be worth to preallocate it based on that
I mean smth like this:
const keys = Object.keys(css)
const cssToRN = new Array(keys.length)
keys.forEach((prop, i) => {
cssToRN[i] = [prop, css[prop]]
})There was a problem hiding this comment.
Hm! Makes sense. But what about Array instantiation cost ?
| // Collect valid style props for the primitive | ||
| const { styleProps } = splitProps(tag, this.props) | ||
| // Collect valid props for the primitive | ||
| const primitiveProps = getPrimitiveProps(tag, this.props) |
There was a problem hiding this comment.
getPrimitiveProps returns fresh object, would be better to reuse it as props which get passed down instead of creating a second object immediately by spreading it
I mean this:
const primitiveProps = getPrimitiveProps(tag, this.props)
primitiveProps.style = getStyles(rnStyles, this.props, styleProps, this.context)
primitiveProps.children = this.props.children
return React.createElement(primitives[tag], primitiveProps)There was a problem hiding this comment.
This will be merged to splitProps instead. Like this -
const { styledProps, primitiveProps } = splitProps(tag, this.props)
| @@ -0,0 +1,3 @@ | |||
| import { emotionPrimitive } from './emotionPrimitive' | |||
There was a problem hiding this comment.
u can shorten it to just
export { emotionPrimitive } from './emotionPrimitive'There was a problem hiding this comment.
also it seems that u should export a default here (instead of named export) with
export { emotionPrimitive as default } from './emotionPrimitive'
|
|
||
| function isValidStyleProp(element, propName) { | ||
| if (element === 'Text') { | ||
| return textStyleProps.includes(propName) |
There was a problem hiding this comment.
includes requires polyfills to be used by library's users if they want to support older browsers, not sure if it's worth using in that case as indexOf equivalent is just a little bit more verbose
There was a problem hiding this comment.
Agree! I will replace this with indexOf > -1
Please make sure that you have checked "Allow edits from maintainers." checkbox here - I will try to find some time to add needed changes to this PR tomorrow. This doesn't have to block this PR though - it's just a small optimization that can be done. |
|
Cool! I was thinking of removing react-emotion first from this package but realised that may be we can bundle accordingly! Thanks. |
| if (stringMode === true && strings[i + 1] !== undefined) { | ||
| styles += strings[i + 1] | ||
| } | ||
| }, this) |
There was a problem hiding this comment.
wouldnt arrow function be good enough here? passing this like that is a little bit surprising for the reader at the first moment
There was a problem hiding this comment.
It was to me too when I first referenced this with emotion/css. We can change this, no issues.
There was a problem hiding this comment.
I did this to save a few bytes so that Babel didn’t need to add var _this = this. It won’t make a big difference but it helps. I’m fine with changing it though
There was a problem hiding this comment.
if that's the case I'm fine with keeping it here too
| } | ||
|
|
||
| function convertStyles(str) { | ||
| if (typeof str === 'string' && str.length === 0) return str |
There was a problem hiding this comment.
we could return '' here, i doubt it is any different than returning str here which is an empty string in that case and making it more explicit would help recognizing the intention a little bit sooner (not that it's particularly hard to see it in current form, but still 🤷♂️ )
| // Get prop name and prop value | ||
| const ar = style.split(': ') | ||
|
|
||
| if (ar[0] && ar[1]) { |
There was a problem hiding this comment.
both need to be coerced to booleans, we could make this more explicit by checking ar.length === 2
There was a problem hiding this comment.
Nope. Earlier I was checking if both were not undefined!
There was a problem hiding this comment.
how those elements could be undefined? checking against .length here is preferable as we should avoid out of bounds accesses
| import { css } from './css' | ||
|
|
||
| function isPrimitiveComponent(component: React.ElementType) { | ||
| switch (component) { |
There was a problem hiding this comment.
is switch case going to be faster than indexOf or object property lookup?
There was a problem hiding this comment.
Yes. switch cases are realllllllllllllly fast.
There was a problem hiding this comment.
May be object property lookup!?
There was a problem hiding this comment.
We can't do a object property lookup since our keys are functions but switch cases are wayyyyyyyyyyyyyy faster anyway. https://esbench.com/bench/5b3f224bf2949800a0f61e14
|
|
||
| return React.createElement( | ||
| component, | ||
| pickAssign(pick, {}, props, { |
| this.setState({ theme }) | ||
| } | ||
|
|
||
| export const testAlwaysTrue = () => true | ||
|
|
||
| export const pickAssign = function(testFn, target) { | ||
| export const pickAssign: ( |
There was a problem hiding this comment.
would be good to package those common utils at some point
| buffer += interpolation | ||
| export function css(...args: any) { | ||
| let vals | ||
| styles = [] |
There was a problem hiding this comment.
we could maybe reuse styles array instead of creating a new one that has to be GCed?
There was a problem hiding this comment.
I'm not sure if it's really worth it/will make a difference. We create a bunch of arrays since css-to-react-native accepts styles like this [['color', 'hotpink'], ['background-color', 'yellow']] so I don't thinking creating one less here will matter. Also, I don't think mutating it is a good idea since StyleSheet.flatten could have memoization/could have it in the future and using the same array would break that.(that might never happen but in general I think passing an object to a function that we don't control and mutating it isn't a great idea)
| @@ -3,71 +3,82 @@ import transform from 'css-to-react-native' | |||
| import { StyleSheet } from 'react-primitives' | |||
| import { interleave } from './utils' | |||
|
|
|||
| export function css(...args: any) { | |||
| let vals | |||
| let styles | |||
There was a problem hiding this comment.
hm, i guess this is most efficient way to do this, but this code (as well as similar code in original emotion) is really hard to follow because of this leaking state, maybe we could at least comment those lines here (state declaration) and its initialization in css?
|
Did |
|
Yeah, one sec, I'll fix it. |
|
Sorry I accidentally used |
|
All good! I did that on master a few days ago 😂 |
|
I'm gonna rename this to |
|
Just published |
What: Add
emotion-primitivesto emotion monorepoWhy:
How: Integration with React primitives
Checklist:
TODO
withComponentand etc for React Native and SketchOnce this is done, we can extract parts for React Native and create a separate package
emotion-native.