Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simple, robust and performant API #597

Closed
wants to merge 28 commits into from
Closed
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

simplify render props apis

  • Loading branch information...
rofrischmann
rofrischmann committed Sep 20, 2018
commit b3f24deccb9e31c565320ee6c6ed163067e83fcb
@@ -1,35 +1,27 @@
/* @flow */
import resolveRule from './resolveRule'

export default function FelaComponentFactory(
createElement: Function,
FelaTheme: Function,
contextTypes?: Object
): Function {
function FelaComponent(
{ children, customClass, render = 'div', rule, style, ...restProps },
{ children, as = 'div', style, ...otherProps },
{ renderer }
) {
return createElement(FelaTheme, {
render: theme => {
const props = rule ? { theme, ...restProps } : theme

const className = `${
customClass ? `${customClass} ` : ''
}${renderer._renderStyle(
resolveRule(rule || style, props, renderer),
props
)}`
return createElement(FelaTheme, undefined, theme => {
const className =
style instanceof Function
? renderer.renderRule(style, { theme, ...otherProps })
: renderer._renderStyle(style, otherProps)

if (render instanceof Function) {
return render({
className,
theme,
})
}
if (children instanceof Function) {
return children({
className,
theme,
})
}

This comment has been minimized.

Copy link
@TxHawks

TxHawks Sep 20, 2018

Collaborator

I assume there are quite some people already using the FelaComponent API. The breaking changes to its API, while good, will require quite an elaborate process from users. I large code bases, this can be an issue.

Are you planning a codemod?

This comment has been minimized.

Copy link
@weserio

weserio Sep 20, 2018

Author Owner

Yeah definitely. This is not going to ship without either fallback support (including deprecation warnings) or codemods =)

This comment has been minimized.

Copy link
@TxHawks

TxHawks Sep 20, 2018

Collaborator

When it's time, we have quite a large codebase that makes extensive use of both the HoC APIs and the render-prop api, so happy to help test codemods

This comment has been minimized.

Copy link
@weserio

weserio Sep 20, 2018

Author Owner

That'd be awesome!
One thing I stumbled over I'm not 100% sure how to convert is the following:

const Button = createComponent(({ color }) => ({
  color,
  fontSize: '15px'
})

const ExtendedButton = createComponent(({ bgColor }) => ({
  backgroundColor: bgColor,
  fontSize: '18px',
  lineHeight: 1.2
}, Button)

// => color:red;background-color:blue;font-size:18px;line-height:1.2
<ExtendedButton color="red" bgColor="blue" />

How would you implement composition with FelaComponent? Of course you can explicitly accept e.g. extend or sth. but that seems like recuring boilerplate to me.
If we figure out user-friendly composition with render-props, I'm all in for replacing everything.

We can discuss that on Gitter if you want to!

This comment has been minimized.

Copy link
@TxHawks

TxHawks Sep 20, 2018

Collaborator

react-adopt seems to be the most popular solution for render-prop composition.

Right now, we use the as prop in a similar way to the second argument of createComponent, but only in the string for html primitive element case. What if we also allowed components there and automatically compose?

This comment has been minimized.

Copy link
@weserio

weserio Sep 20, 2018

Author Owner

Ok, I think I got it...
We can actually pass style/rule to the render-props too.

// Button.js
const button = ({ color }) => ({
  color,
  fontSize: '15px'
})

const Button = props => <FelaComponent style={button} {...props} />
// ExtendedButton.js
import Button from './Button'

const extendedButton = ({ bgColor }) => ({
  backgroundColor: bgColor,
  fontSize: '18px',
  lineHeight: 1.2
})

const ExtendedButton = props => (
  <Button>
    {renderProps => <FelaComponent style={combineRules(renderProps.style, extendedButton)} />}
  </Button>
)

// => color:red;background-color:blue;font-size:18px;line-height:1.2
<ExtendedButton color="red" bgColor="blue" />

Only issue here: Button will render it's className even though its never used here.
Another way would be to export the button style of course, but I'd rather compose components then styles.

We could even go add allow arrays for style and apply combineRules by default.

This comment has been minimized.

Copy link
@TxHawks

TxHawks Sep 20, 2018

Collaborator

And it is a bit harder to understand because none of the implementation detail is abstracted away. That's both good and bad, I guess.

Just thinking out loud here, but maybe if style is an array, its items can be automatically be passed to combineRules?

This comment has been minimized.

Copy link
@TxHawks

TxHawks Sep 20, 2018

Collaborator

Just realized - the renderProps object in the example above doesn't have a style property, only className and theme

This comment has been minimized.

Copy link
@weserio

weserio Sep 21, 2018

Author Owner
  1. Yeah, I think we could expose the array-syntax with combineRules by default.
  2. Yep the renderProps right now only has className and theme, we would have to expose that, but I got a new idea anyways:
// ExtendedButton.js
import Button from './Button'

const extendedButton = ({ bgColor }) => ({
  backgroundColor: bgColor,
  fontSize: '18px',
  lineHeight: 1.2
})

const ExtendedButton = props => <Button extend={extendedButton} {...props} />

// => color:red;background-color:blue;font-size:18px;line-height:1.2
<ExtendedButton color="red" bgColor="blue" />

This comment has been minimized.

Copy link
@weserio

weserio Sep 21, 2018

Author Owner

Ok. This API is freaking me out. It basically solves every issue and yet has the flexibility of createComponent.

This comment has been minimized.

Copy link
@brianespinosa

brianespinosa Sep 21, 2018

Contributor

Yes I like that. And unless I am missing something, it seems like it also solves the issue of adding lots of additional components to the React render tree.


return createElement(render, { className }, children)
},
return createElement(as, { className }, children)
})
}

@@ -32,7 +32,7 @@ export default function FelaThemeFactory(
}

render() {
return this.props.render(this.state.theme)
return this.props.children(this.state.theme)
}
}

@@ -1,17 +1,13 @@
import 'raf/polyfill'
import React, { Component, createElement } from 'react'
import PropTypes from 'prop-types'
import { mount } from 'enzyme'
import toJson from 'enzyme-to-json'
import { css } from 'js-beautify'

import { renderToString } from 'fela-tools'
import { createRenderer } from 'fela'

import FelaThemeFactory from '../FelaThemeFactory'
import FelaComponentFactory from '../FelaComponentFactory'
import createTheme from '../createTheme'
import { THEME_CHANNEL } from '../themeChannel'

import createSnapshot from '../__helpers__/createSnapshot'

const FelaTheme = FelaThemeFactory(Component, {
[THEME_CHANNEL]: PropTypes.object,
})
@@ -22,172 +18,95 @@ const FelaComponent = FelaComponentFactory(createElement, FelaTheme, {
})

describe('Using the FelaComponent component', () => {
it('correctly render a fela rule', () => {
const renderer = createRenderer()

const wrapper = mount(
<FelaComponent
style={{
fontSize: '12px',
color: 'red',
}}
render={({ className }) => (
<div className={className}>I am red and written in 12px.</div>
)}
/>,
{
context: {
renderer,
},
}
)

expect([css(renderToString(renderer)), toJson(wrapper)]).toMatchSnapshot()
})

it('correctly concat "customClass" with className', () => {
const renderer = createRenderer()

const wrapper = mount(
<FelaComponent
customClass="custom-class"
style={{
color: 'red',
}}
render={({ className }) => (
<div className={className}>I am red and have a custom class.</div>
)}
/>,
{
context: {
renderer,
},
}
)

expect([css(renderToString(renderer)), toJson(wrapper)]).toMatchSnapshot()
})

it('correctly pass the theme to the "style" prop', () => {
const themeContext = createTheme({
fontSize: '15px',
})

const renderer = createRenderer()

const wrapper = mount(
<FelaComponent
style={theme => ({
fontSize: theme.fontSize,
color: 'red',
})}
render={({ className, theme }) => (
<div className={className}>
I am red and written in {theme.fontSize}.
</div>
)}
/>,
{
context: {
[THEME_CHANNEL]: themeContext,
renderer,
},
}
)

expect([css(renderToString(renderer)), toJson(wrapper)]).toMatchSnapshot()
it('should correctly render a fela rule', () => {
expect(
createSnapshot(
<FelaComponent
style={{
fontSize: '12px',
color: 'red',
}}>
{({ className }) => (
<div className={className}>I am red and written in 12px.</div>
)}
</FelaComponent>
)
).toMatchSnapshot()
})

it('correctly pass the theme and other props to the "rule" prop', () => {
const themeContext = createTheme({
fontSize: '15px',
it('should correctly pass the theme and other props to functional style', () => {
const rule = ({ theme, bgc }) => ({
fontSize: theme.fontSize,
backgroundColor: bgc || 'red',
})

const renderer = createRenderer()

const wrapper = mount(
<FelaComponent
bgc="blue"
rule={({ theme, bgc }) => ({
fontSize: theme.fontSize,
backgroundColor: bgc || 'red',
})}
render={({ className, theme }) => (
<div className={className}>
I am red and written in {theme.fontSize}.
</div>
)}
/>,
{
context: {
[THEME_CHANNEL]: themeContext,
renderer,
},
}
)

expect([css(renderToString(renderer)), toJson(wrapper)]).toMatchSnapshot()
expect(
createSnapshot(
<FelaComponent style={rule} bgc="blue">
{({ className, theme }) => (
<div className={className}>
I am red and written in {theme.fontSize}.
</div>
)}
</FelaComponent>,
{ fontSize: '15px' }
)
).toMatchSnapshot()
})

it('should default to a div', () => {
const renderer = createRenderer()

const wrapper = mount(
<FelaComponent
style={{
fontSize: '12px',
color: 'red',
}}
/>,
{
context: {
renderer,
},
}
)

expect([css(renderToString(renderer)), toJson(wrapper)]).toMatchSnapshot()
expect(
createSnapshot(
<FelaComponent
style={{
fontSize: '12px',
color: 'red',
}}
/>
)
).toMatchSnapshot()
})

it('should render children in default mode', () => {
const renderer = createRenderer()

const wrapper = mount(
<FelaComponent
style={{
fontSize: '12px',
color: 'red',
}}>
<span>Hello World</span>
</FelaComponent>,
{
context: {
renderer,
},
}
)

expect([css(renderToString(renderer)), toJson(wrapper)]).toMatchSnapshot()
expect(
createSnapshot(
<FelaComponent
style={{
fontSize: '12px',
color: 'red',
}}>
<span>Hello World</span>
</FelaComponent>
)
).toMatchSnapshot()
})

it('should accept a string primitive as render target', () => {
const renderer = createRenderer()

const wrapper = mount(
<FelaComponent
style={{
fontSize: '12px',
color: 'red',
}}
render="span"
/>,
{
context: {
renderer,
},
}
)
it('should accept a string primitive type via as-prop', () => {
expect(
createSnapshot(
<FelaComponent
as="span"
style={{
fontSize: '12px',
color: 'red',
}}
/>
)
).toMatchSnapshot()
})

expect([css(renderToString(renderer)), toJson(wrapper)]).toMatchSnapshot()
it('should render children using the correct as-prop', () => {
expect(
createSnapshot(
<FelaComponent
as="h1"
style={{
fontSize: '12px',
color: 'red',
}}>
Hello World
</FelaComponent>
)
).toMatchSnapshot()
})
})
@@ -1,31 +1,25 @@
import 'raf/polyfill'
import React, { Component } from 'react'
import PropTypes from 'prop-types'
import { mount } from 'enzyme'
import toJson from 'enzyme-to-json'

import FelaThemeFactory from '../FelaThemeFactory'
import createTheme from '../createTheme'
import { THEME_CHANNEL } from '../themeChannel'

import createSnapshot from '../__helpers__/createSnapshot'

const FelaTheme = FelaThemeFactory(Component, {
[THEME_CHANNEL]: PropTypes.object,
})

describe('Using the FelaTheme component', () => {
it('correctly pass the theme down', () => {
const themeContext = createTheme({
color: 'red',
})

const wrapper = mount(
<FelaTheme render={theme => <div>The color is {theme.color}.</div>} />,
{
context: {
[THEME_CHANNEL]: themeContext,
},
}
)

expect(toJson(wrapper)).toMatchSnapshot()
expect(
createSnapshot(
<FelaTheme>
{theme => <div>The color is {theme.color}.</div>}
</FelaTheme>,
{ color: 'red' }
)
).toMatchSnapshot()
})
})
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.