export emotion-theming from react-emotion #475

quantizor · 2017-11-27T05:14:23Z

While refactoring for a proof of concept, it quickly became cumbersome to have to split up the import just to pull in theming HOCs.

Since theming functionality is partially built-in by default in emotion v8 via supporting p => p.theme etc, I think it makes sense to go the rest of the way and re-export emotion-theming for ease of use.

The package itself should probably go away for emotion v9.

Checklist:

Documentation
Tests
Code complete

quantizor · 2017-11-30T23:51:48Z

@tkh44 @mitchellhamilton hey folks, any comments on this?

quantizor · 2017-12-15T05:42:11Z

From a somewhat selfish standpoint, this is a soft blocker on an internal migration away from styled-components at my company... trying to make the switch as painless as possible.

Out of curiosity, why the hesitation to merge this based on our previous twitter convo @tkh44?

tkh44 · 2017-12-17T05:23:19Z

Sorry, I've been on vacation for the last few weeks.

@mitchellhamilton and I have talked about this about a week ago and we both were leaning on not merging. After some thought I think this might make sense just from a developer experience pov. Let's remove as much friction as possible for devs onboarding with emotion.

@mitchellhamilton when we talked you mentioned some concerns with the preact build. Can you elaborate here? What are the other cons?

quantizor · 2017-12-17T06:28:28Z

After some thought I think this might make sense just from a developer experience pov

Very much agree. When I was refactoring it felt weird to consume theming from a separate package — like it was disconnected in some way. But that really isn’t the case since it relies on certain static variables being available, etc.

emmatown · 2017-12-17T08:05:02Z

emotion-theming requires React.Children.only so it requires preact-compat whereas preact-emotion currently only requires preact so exporting emotion-theming from preact-emotion would mean it would require an alias from react to preact-compat.

I think what we could do is make emotion-theming export two functions, createWithTheme and createThemeProvider which accept a React-like API and a channel, they would return what their names imply. We would then export the created versions from react-emotion and preact-emotion. For the React.Children.only thing, we could check if Children exists and return props.children[0] if it doesn't exist.

quantizor · 2017-12-17T15:22:12Z

I could just revise emotion-theming. Children.only is essentially just a check to prevent an array of children, so shouldn’t be challenging to replace that check with a simpler alternative.

tkh44 · 2017-12-17T18:45:37Z

packages/emotion-theming/src/theme-provider.js

@@ -77,8 +77,18 @@ class ThemeProvider extends Component {
  render() {
    if (!this.props.children) {
      return null
+    } else if (Array.isArray(this.props.children)) {


One trick I stole from preact-compat is to coerce it into an array.

https://github.com/tkh44/holen/blob/master/src/index.js#L4

Another option is to just not worry about it except for a warning. For React > 16 it wouldn't matter anyway, right?

Yeah for 16+ it wouldn’t matter.

I went ahead and updated the impl to be simpler :) along the lines of your suggestion

quantizor · 2017-12-21T06:19:19Z

Any more notes?

emmatown · 2017-12-21T06:31:47Z

In my comment before, I should have made it more clear that this would mean preact-emotion requires an alias from react to preact. While most preact apps have an alias from react to preact-compat, it would mean that preact wouldn't be a first class citizen in preact-emotion, this was why I was suggesting having factories. The other option would be to make emotion-theming a private module and bundle it in preact-emotion and react-emotion with react aliased to preact in the preact-emotion build. IMO factories make more sense because they'll fit with #464.

quantizor · 2017-12-21T06:37:13Z

Gotcha. Okay I’ll work on this over the next few days 👍

codecov · 2017-12-21T11:49:03Z

Codecov Report

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

Impacted Files	Coverage Δ
packages/react-emotion/src/index.js	`100% <ø> (ø)`	⬆️
packages/emotion-theming/src/theme-provider.js	`100% <100%> (ø)`	⬆️
packages/emotion-theming/src/with-theme.js	`100% <100%> (ø)`	⬆️
packages/emotion-theming/src/index.js	`100% <100%> (ø)`
packages/emotion-theming/src/factory.js	`100% <100%> (ø)`

While refactoring for a proof of concept, it quickly became cumbersome to have to split up the import just to pull in theming imports. Since theming functionality is partially built-in by default in emotion v8 via supporting `p => p.theme` etc, I think it makes sense to go the rest of the way and re-export `emotion-theming` for ease of use. update snapshots since the file contents changed

quantizor · 2017-12-22T05:57:52Z

@mitchellhamilton okay, I converted withTheme and ThemeProvider into factory functions... but I don't really understand how babel-macro works. Mind giving some guidance on how to get the Preact part done?

emmatown · 2017-12-22T07:10:35Z

@probablyup babel-macros isn't related to this. First, could you remove createTheming and change it to be separate factories, having them as separate factories means we can add the pure flag so they can be individually removed if they're not used. I should have said this before but could you base this on the instances branch and only export the factories from emotion-theming and then in react-emotion and preact-emotion create and export withTheme and emotionTheming. Also, I know before I said to have a channel option but I think let's just make the only option be the React-like library, especially if we change things in the future because of createContext and etc.

UPDATE: instances is now on master

quantizor · 2018-01-02T23:26:02Z

I'm not going to be able to work on this for a while, so just going to close.

tkh44 reviewed Dec 17, 2017

View reviewed changes

evan-scott-zocdoc and others added 2 commits December 22, 2017 00:43

refactor out usage of React.Children.only

5f17bf0

convert emotion-theming exports into factory-style constructors

3b9f0a2

quantizor closed this Jan 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

export emotion-theming from react-emotion #475

export emotion-theming from react-emotion #475

quantizor commented Nov 27, 2017 •

edited

Loading

quantizor commented Nov 30, 2017

quantizor commented Dec 15, 2017

tkh44 commented Dec 17, 2017

quantizor commented Dec 17, 2017

emmatown commented Dec 17, 2017

quantizor commented Dec 17, 2017 via email •

edited

Loading

tkh44 Dec 17, 2017

tkh44 Dec 17, 2017

tkh44 Dec 17, 2017

quantizor Dec 18, 2017

quantizor commented Dec 21, 2017

emmatown commented Dec 21, 2017

quantizor commented Dec 21, 2017

codecov bot commented Dec 21, 2017 •

edited

Loading

quantizor commented Dec 22, 2017

emmatown commented Dec 22, 2017 •

edited

Loading

quantizor commented Jan 2, 2018

export emotion-theming from react-emotion #475

export emotion-theming from react-emotion #475

Conversation

quantizor commented Nov 27, 2017 • edited Loading

quantizor commented Nov 30, 2017

quantizor commented Dec 15, 2017

tkh44 commented Dec 17, 2017

quantizor commented Dec 17, 2017

emmatown commented Dec 17, 2017

quantizor commented Dec 17, 2017 via email • edited Loading

tkh44 Dec 17, 2017

Choose a reason for hiding this comment

tkh44 Dec 17, 2017

Choose a reason for hiding this comment

tkh44 Dec 17, 2017

Choose a reason for hiding this comment

quantizor Dec 18, 2017

Choose a reason for hiding this comment

quantizor commented Dec 21, 2017

emmatown commented Dec 21, 2017

quantizor commented Dec 21, 2017

codecov bot commented Dec 21, 2017 • edited Loading

Codecov Report

quantizor commented Dec 22, 2017

emmatown commented Dec 22, 2017 • edited Loading

quantizor commented Jan 2, 2018

quantizor commented Nov 27, 2017 •

edited

Loading

quantizor commented Dec 17, 2017 via email •

edited

Loading

codecov bot commented Dec 21, 2017 •

edited

Loading

emmatown commented Dec 22, 2017 •

edited

Loading