Emotion Native #759

nitin42 · 2018-07-09T11:14:44Z

What: Styling Native components using emotion and a new primitives-core package for shared utils.

Why:

How: This PR adds support for styling React Native components using emotion, adds a new package primitives-core for primitive shared utils and also refactors the primitives package.

Checklist:

✅ Documentation
✅ Tests
✅ Code complete

Related #755

nitin42 · 2018-07-09T17:51:42Z

Almost done! Just need some work on flow types.

nitin42 · 2018-07-09T17:58:43Z

I don't know why there are some flow types error in other packages (I haven't touch any). Plus, typescript tests seems to be broken.

emmatown

This is looking great!!

emmatown · 2018-07-09T21:57:23Z

package.json

-  "version": "7.0.13",
-  "scripts": {
-    "build:watch": "node scripts/build/watch",
-    "build": "node scripts/build",


Would you be able to revert these formatting changes in the package.jsons?

emmatown · 2018-07-09T22:01:29Z

packages/primitives-core/package.json

@@ -0,0 +1,26 @@
+{
+	"name": "primitives-core",


This should be @emotion/primitives-core and it should be imported as @emotion/primitives-core

emmatown · 2018-07-09T22:02:32Z

packages/primitives/package.json

 		"babel-plugin-emotion": "^9.2.5",
-		"css-to-react-native": "^2.2.0"
+		"primitives-core": "1.0.0-beta"


@emotion/primitives-core

emmatown · 2018-07-09T22:04:17Z

packages/primitives/test/css.test.js

@@ -1,5 +1,5 @@
 // @flow
-import { css } from '@emotion/primitives'
+import { css } from '../src'


This should be @emotion/primitives, all package names are aliased to their src.

emmatown · 2018-07-09T22:04:40Z

packages/primitives/test/emotion-primitives.test.js

@@ -8,7 +8,7 @@ import reactPrimitives from 'react-primitives'
 import { ThemeProvider } from 'emotion-theming'
 import { render, unmountComponentAtNode } from 'react-dom'

-import styled from '@emotion/primitives'
+import styled from '../src'


Same as above

emmatown · 2018-07-09T22:05:14Z

packages/primitives/test/no-babel/basic.test.js

@@ -1,7 +1,7 @@
 // @flow
 import '../mock-primitives'
 import * as React from 'react'
-import styled, { css } from '@emotion/primitives'
+import styled, { css } from '../../src'


Same as above

emmatown · 2018-07-09T22:09:10Z

packages/native/package.json

@@ -0,0 +1,23 @@
+{
+	"name": "emotion-native",


@emotion/native

emmatown

🎉

emmatown · 2018-07-13T11:38:54Z

packages/native/src/index.js

+
+const css = createCss(reactNative.StyleSheet)
+
+const components = `ActivityIndicator ActivityIndicatorIOS ART Button DatePickerIOS DrawerLayoutAndroid


Could you make this into an array since it's easier to read and there's no need to split

Sure! No issues.

emmatown · 2018-07-13T11:40:36Z

packages/native/test/native-css.test.js

+
+let returnArguments = (...args) => args
+
+describe('Emotion native css', () => {


I'm not really sure if we need all these tests duplicated, maybe it could just be a small smoke test to make sure css works

I was thinking of this too! I'll need some help over here so will ping you later when I comeback to this after fixing basic stuff.

emmatown · 2018-07-13T11:41:46Z

packages/primitives-core/README.md

+
+## Introduction
+
+This package contains some shared utilities which are consumed by `@emotion/primitives` and `@emotion/native` for styling and rendering components.


Could you add a note saying that most people won't need this package and should use @emotion/primitives and @emotion/native?

emmatown · 2018-07-13T11:44:44Z

packages/primitives-core/src/styled.js

+
+export function createStyled(
+  StyleSheet: Object,
+  getShouldForwardProp: Function = (cmp: React.ElementType): Function =>


This should be getShouldForwardProp: (cmp: React.ElementType) => (prop: string) => boolean = () => defaultPickTest and could you make this argument into an object so that if there needs to be more options in the future it's easier?

Yup! I'll fix it

emmatown · 2018-07-13T11:45:42Z

packages/primitives/src/styled.js


-function isPrimitiveComponent(component: React.ElementType) {
+function shouldForwardProp(component: React.ElementType) {


Should be called getShouldForwardProp

emmatown · 2018-07-13T11:46:55Z

packages/primitives/test/emotion-primitives.test.js

+  //   render(<Text innerRef={ref} id="something" />, rootNode)
+  //   expect(ref.current).toBe(rootNode.querySelector('#something'))
+  //   unmountComponentAtNode(rootNode)
+  // })


Any reason this was commented out?

Hm.. can't recall may be its related to emotion primitives PR work. Anyway, I'll uncomment and update the test.

emmatown · 2018-07-13T11:49:05Z

packages/primitives-core/README.md

+
+### `createStyled(StyleSheet)`
+
+`createStyled` also accepts a platform specific `StyleSheet` method for creating styles, and returns a styled component. You can assign primitives to it for example - `View`


"returns a styled component", createStyled returns a function that returns a function that returns a styled component right?

emmatown · 2018-07-13T11:49:58Z

packages/native/README.md

+
+## Theming
+
+Use `emotion-theming` for theming support.


An example maybe?

Hm... That'd be great! I'll add it

emmatown · 2018-07-13T11:52:07Z

packages/native/README.md

+AppRegistry.registerComponent('ExampleApp', () => App);
+```
+
+The API surface remains same for `@emotion/native` which means you can use all the API methods similar to `emotion`.


Reading this is confusing, the same as what? emotion? or @emotion/primitives? and you can't use everything that emotion has, e.g. keyframes and injectGlobal

Oops! I missed those parts of emotion API. I'll fix it.

nitin42 · 2018-07-13T12:07:44Z

I also noticed some flow types errors for CreateStyled in @emotion on running yarn flow.

emmatown · 2018-07-13T12:17:08Z

if you merge from master, those flow errors should be fixed.

nitin42 · 2018-07-14T16:57:22Z

I pulled latest changes from master and now getting this error on running yarn flow 😅

.flowconfig:14 Unsupported config section: "declarations"

Any ideas ?

Except this, all tests are updated and are passing.

emmatown · 2018-07-14T20:53:22Z

@nitin42 run yarn install

nitin42 · 2018-07-15T06:48:30Z

Still no luck! Getting that same error on running yarn install plus tests seems to be broken when I pulled the changes from master. Can you fix this and then I pull the updates ?

nitin42 · 2018-07-17T13:58:35Z

Thanks @tkh44 for the review!

Andarist · 2018-07-17T18:16:55Z

packages/native/package.json

+	"dependencies": {
+		"@emotion/primitives-core": "1.0.0"
+	},
+	"peerDependencies": {


shouldnt a peer on react be here?

Oops! Done ✅

Andarist · 2018-07-17T18:21:40Z

packages/native/README.md

+	color: hotpink;
+`;
+
+export default class App extends React.Component {


indentation is different here than in other examples (in this file), this could be enforced with prettier + lint-staged on precommit hook (with husky) as it can handle .md too, cc @mitchellhamilton

That'd be great. This will also resolve all the formatting issues!

@Andarist If you could submit a PR for it, that would be great!

I'll try to remember to do that later :)

Andarist · 2018-07-17T18:30:11Z

packages/native/src/index.js

+
+export { css }
+
+export default /* #__PURE__ */ styled


this doesnt make much sense here, #__PURE__ comments are useful for call expressions, this is just exported identifier

Not sure what actual value it gives in context of react-native (it's not that much bytes-constrained as web), it certainly won't hurt, but would have to be used differently.

I would convert this to:

export default components.reduce((acc, comp) => Object.defineProperty(acc, comp, { enumerable: true, configurable: false, get() { return styled(reactNative[comp]) } }) , styled)

This would make it self contained expression that can be more safely annotated with PURE.

You could also use my plugin to insert those PURE annotations automatically - https://github.com/Andarist/babel-plugin-annotate-pure-calls , but ofc that's not obligatory 😉

That makes sense. I'll refactor the code to use your solution.

Andarist · 2018-07-17T18:30:30Z

packages/native/src/index.js

+
+import { styled } from './styled'
+
+const css = createCss(reactNative.StyleSheet)


this could be annotated with PURE

Andarist · 2018-07-17T18:30:59Z

packages/native/src/styled.js

+/**
+ * a function that returns a styled component which render styles in React Native
+ */
+let styled = createStyled(StyleSheet)


this is also a good candidate for PURE

are there any side-effects of using PURE here ?

Andarist · 2018-07-17T18:34:44Z

packages/primitives-core/README.md

+
+### `createStyled(StyleSheet)`
+
+`createStyled` also accepts a platform specific `StyleSheet` method for creating styles, and returns a function that returns a function that returns a styled component. You can assign primitives to it for example - `View`


and returns a function that returns a function that returns a styled component

while this is technically correct, it sounds like a gibberish 😅 could we rephrase it ?

Andarist · 2018-07-17T18:36:42Z

packages/primitives-core/src/styled.js

+
+export function createStyled(
+  StyleSheet: Object,
+  { getShouldForwardProp = () => defaultPickTest }: options = {}


I've noticed that this API differs from web one. Regular emotion accepts shouldForwardProp - is there any reason why this is different here?

the whole createStyled also differs from regular create-emotion-styled API which accepts whole emotion (created upfront with create-emotion). It seems to me that react-native equivalent would be

const css = createCss(StyleSheet) const styled = createStyled(css)

or

const css = createCss(StyleSheet) const styled = createStyled({ css })

I'm not sure if native version has some other API than css/styled (web one has), so I'm not sure which one would be more appropriate here.

regarding shouldForwardProp - I think I've found the answer, this is used at 1 layer above than regular shouldForwardProp, right? would be good to support normal shouldForwardProp, but it can also be added later (maybe create some kind of TODO list of things that could be added to achieve parity with regular emotion)

The API here differs because, we may add some more options along with getShouldForwardProp in the future.

The API here differs because, we may add some more options along with getShouldForwardProp in the future.

Making it an object is ofc preferrable, but that was not the question. Normal emotion also accepts configuration objects - my question was about API differences.

And what about 1st arg to createStyled?

There are no such API differences except we use StyleSheet constructor and a little refactored code taken from @emotion/primitives. The goal is to keep the signature of functions similar across the env. whether it's web, native or sketch. I liked your suggestion and will make the changes.

Your argument is valid here since having such API reduces the maintenance cost.

Andarist · 2018-07-17T18:41:27Z

packages/primitives-core/src/styled.js

+) {
+  const css = createCss(StyleSheet)
+
+  return function createEmotion(component: React.ElementType) {


on web this is called createEmotionStyled, while returned function is called createStyled

Hm... it's a little inconsistent with the nomenclature. I'll fix that later!

Andarist · 2018-07-17T18:43:57Z

packages/primitives-core/src/styled.js

+
+      Styled.displayName = `emotion(${getDisplayName(component)})`
+
+      return Styled


would be cool to support component as selectors, not sure if possible though and can be added later if yes

I haven't looked into it yet to check whether its possible or not. Let's save it up for later!

Andarist · 2018-07-17T18:44:35Z

packages/primitives/src/index.js

-import { createStyled } from './styled'
+import { styled as createStyled } from './styled'
+
+const css = createCss(StyleSheet)


this is a good candidate for PURE

nitin42 · 2018-07-18T06:30:59Z

I've made some changes. I am still not sure about annotating call expressions with PURE because it may have side-effects ? Still not sure. Let me know! 😅

Thanks for the review!

emmatown · 2018-07-18T07:16:07Z

The pure comment tells uglify that a function call can be removed if the return value is not used so it doesn't really matter whether something has side effects or not but rather if they have side effects that are useful without the return value.

For example, on the web, emotion's css isn't pure since it inserts css rules and etc. but we add the pure comment in babel-plugin-emotion in front of css calls since if you're not using the class name from css, the call is pointless.

nitin42 · 2018-07-26T19:55:50Z

I think we can merge this now. The remaining changes don't affect the API design!

This is the same peerDep that glamorous-native has.

codecov · 2018-07-28T22:38:39Z

Codecov Report

Merging #759 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted Files	Coverage Δ
packages/primitives-core/src/utils.js	`100% <ø> (ø)`
packages/primitives/src/index.js	`100% <100%> (ø)`	⬆️
packages/native/src/styled.js	`100% <100%> (ø)`
packages/primitives/src/styled.js	`100% <100%> (ø)`	⬆️
packages/native/src/index.js	`100% <100%> (ø)`
packages/primitives-core/src/styled.js	`100% <100%> (ø)`
packages/primitives-core/src/css.js	`100% <100%> (ø)`

nitin42 · 2018-07-31T18:54:32Z

@mitchellhamilton have you published the package ? Haha someone jumped in my Twitter DM to ask about our work on emotion native 😅

emmatown · 2018-08-01T02:10:19Z

@nitin42 Yup, it’s published!

Init commit

6b9f575

nitin42 changed the title ~~Init commit~~ Emotion Native Jul 9, 2018

nitin42 added 3 commits July 9, 2018 23:04

add emotion-native

91380c2

remove unused imports

38d3012

added basic types

db3d964

nitin42 added 2 commits July 9, 2018 23:25

fix flow types

d545607

add jsdom env when testing

6ec1ff3

emmatown reviewed Jul 9, 2018

View reviewed changes

Ailrun added the Tag: Enhancement label Jul 11, 2018

nitin42 added 2 commits July 13, 2018 16:30

fixed import statement

94b0c28

fixed import statement and updated package.json

25d75bc

nitin42 added the WIP label Jul 13, 2018

nitin42 added 2 commits July 13, 2018 16:39

added test for Emotion native css/css({})

ae75fe3

added new keywords

b80e73f

nitin42 added the Ready For Review label Jul 13, 2018

nitin42 added 2 commits July 13, 2018 16:53

added documentation for @emotion/native

fc1beb9

added docs for @emotion/primitives-core

ddb2dd2

emmatown reviewed Jul 13, 2018

View reviewed changes

nitin42 added 3 commits July 14, 2018 22:18

fixed docs and flow types

ee2181c

fixed conflicts

f839c70

fixed conflicts

95c28d3

nitin42 added 2 commits July 15, 2018 12:10

fixed conflicts

47f5035

update @emotion/primitives version

fdc1876

Update README.md

df1d259

Andarist reviewed Jul 17, 2018

View reviewed changes

nitin42 added 3 commits July 18, 2018 11:42

add react to peerDependencies

be12b5c

Refactor: default export in native

6a1b33b

fix wording in docs

32b27c1

emmatown added 3 commits July 21, 2018 15:24

Merge branch 'master' into emotion-native

f87c05d

Fix snapshots

975bc68

Merge remote-tracking branch 'upstream/master' into emotion-native

4f3f998

emmatown added 3 commits July 27, 2018 14:57

Merge branch 'master' into emotion-native

4e8ca76

Loosen peerDep on react-native

8c6694e

This is the same peerDep that glamorous-native has.

Merge branch 'master' into emotion-native

84c1bb9

emmatown merged commit 8edf5d3 into emotion-js:master Jul 28, 2018

emmatown added the Tag: Enhancement label Jul 29, 2018

nitin42 deleted the emotion-native branch August 29, 2018 11:05


		const css = createCss(reactNative.StyleSheet)

		const components = `ActivityIndicator ActivityIndicatorIOS ART Button DatePickerIOS DrawerLayoutAndroid


		let returnArguments = (...args) => args

		describe('Emotion native css', () => {


		## Introduction

		This package contains some shared utilities which are consumed by `@emotion/primitives` and `@emotion/native` for styling and rendering components.


		function isPrimitiveComponent(component: React.ElementType) {
		function shouldForwardProp(component: React.ElementType) {


		### `createStyled(StyleSheet)`

		`createStyled` also accepts a platform specific `StyleSheet` method for creating styles, and returns a styled component. You can assign primitives to it for example - `View`


		import { styled } from './styled'

		const css = createCss(reactNative.StyleSheet)


		Styled.displayName = `emotion(${getDisplayName(component)})`

		return Styled

Emotion Native #759

Emotion Native #759

Conversation

nitin42 commented Jul 9, 2018 • edited Loading

nitin42 commented Jul 9, 2018

nitin42 commented Jul 9, 2018

emmatown left a comment

Choose a reason for hiding this comment

emmatown Jul 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

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

emmatown left a 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

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

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

nitin42 Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitin42 commented Jul 13, 2018

emmatown commented Jul 13, 2018

nitin42 commented Jul 14, 2018 • edited Loading

emmatown commented Jul 14, 2018

nitin42 commented Jul 15, 2018 • edited Loading

nitin42 commented Jul 17, 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

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

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

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

Andarist Jul 17, 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

Choose a reason for hiding this comment

nitin42 commented Jul 18, 2018 • edited Loading

emmatown commented Jul 18, 2018

nitin42 commented Jul 26, 2018

codecov bot commented Jul 28, 2018 • edited Loading

Codecov Report

nitin42 commented Jul 31, 2018

emmatown commented Aug 1, 2018

nitin42 commented Jul 9, 2018 •

edited

Loading

emmatown Jul 9, 2018 •

edited

Loading

nitin42 Jul 13, 2018 •

edited

Loading

nitin42 commented Jul 14, 2018 •

edited

Loading

nitin42 commented Jul 15, 2018 •

edited

Loading

Andarist Jul 17, 2018 •

edited

Loading

nitin42 commented Jul 18, 2018 •

edited

Loading

codecov bot commented Jul 28, 2018 •

edited

Loading