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

Use new context API #1894

Merged
merged 44 commits into from Aug 12, 2018

Conversation

Projects
None yet
4 participants
@alexandernanberg
Member

alexandernanberg commented Aug 6, 2018

This is just to get the ball rolling with the new context API

Fixes #1459
Fixes #1655

Pretty happy with how the ThemeProvider turned out, a lot cleaner. I'll continue working on this and hopefully fully landing this PR.

Enzyme does not support the react > 16.3 yet so it's hard to test this until that gets fixed. We probably want to use a polyfill for this as well, like create-react-context unless we drop React < 16.3 support

No hard feelings if you decide not to use this!

@probablyup

This comment has been minimized.

Contributor

probablyup commented Aug 6, 2018

Good start! Let's remove that memoize-one dep that looks to have slipped in.

At this point for 4.0 I think we will just plan to drop old React support and just target 16.4+ to keep things easy.

@alexandernanberg

This comment has been minimized.

Member

alexandernanberg commented Aug 6, 2018

Oh I actually just forgot to use the memoize function, we need to either memoize the getContext method or move the context to state because otherwise we'll provide a new context value each render which will make consumers update when they might not need to.

Cool, nice to save some KBs as well! 👍

@probablyup

This comment has been minimized.

Contributor

probablyup commented Aug 6, 2018

Let's use state and recalculate upon prop change by hard equality

@alexandernanberg

This comment has been minimized.

Member

alexandernanberg commented Aug 6, 2018

Okay, although I think using the memoized approach is a bit cleaner and simpler, because then you don't need to recalculate in getDerivedPropsFromState. But either way is fine

Edit: Btw I think this is really useful info https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#what-about-memoization

@alexandernanberg

This comment has been minimized.

Member

alexandernanberg commented Aug 7, 2018

I think the ThemeProvider should work now 😮

Switched the test to react-testing-library because enzyme wasn't working (no react 16.3+ compability). If you don't want to switch over entirely to react-testing-library we could migrate back later.

Left the memoization for now but should be fairly simple to change if you'd still like that

@@ -110,7 +87,8 @@ class BaseStyledComponent extends Component<*, BaseState> {
generateAndInjectStyles(theme: any, props: any) {
const { attrs, componentStyle, warnTooManyClasses } = this.constructor
const styleSheet = this.context[CONTEXT_KEY] || StyleSheet.master
// const styleSheet = this.context[CONTEXT_KEY] || StyleSheet.master
const styleSheet = StyleSheet.master

This comment has been minimized.

@alexandernanberg

alexandernanberg Aug 7, 2018

Member

I'm unsure what this previously was for, but I think it will be fine just assigning it to Stylesheet.master?

This comment has been minimized.

@probablyup

probablyup Aug 7, 2018

Contributor

@kitten can you comment on this? I think it's okay

This comment has been minimized.

@alexandernanberg

alexandernanberg Aug 8, 2018

Member

Oh just realized that that context comes from the StyleSheetManager, so I'll have to wrap it in a StyleSheetConsumer or similar to get the correct value

return createElement(target, propsForElement)
return (
<ThemeConsumer>
{({ theme } = {}) => {

This comment has been minimized.

@probablyup

probablyup Aug 7, 2018

Contributor

Can you use the empty object from utils/empties?

This comment has been minimized.

@probablyup

probablyup Aug 7, 2018

Contributor

Can we also cache this function somehow instead of recreating it every render?

This comment has been minimized.

@alexandernanberg

alexandernanberg Aug 7, 2018

Member

If I use EMPTY_OBJECT I get a flow error though, so the best way might create a type for the context and not destructure it, right? I've not really worked with flow before so I might need some help on certain bits

Yeah good point, although I don't it's that expensive to recreate it

This comment has been minimized.

@probablyup

probablyup Aug 7, 2018

Contributor

It's more about memory pressure. This is a hot path, so we want it to be as maximally-efficient as possible.

This comment has been minimized.

@alexandernanberg

alexandernanberg Aug 7, 2018

Member

Makes sense, will refactor to take this into account!

getContext(theme: (outerTheme: ?Theme) => void, outerTheme?: Theme) {
return {
theme: this.getTheme(theme, outerTheme),
}

This comment has been minimized.

@alexandernanberg

alexandernanberg Aug 8, 2018

Member

What if we instead of using setting context to { theme } we set it to theme without a wrapping object, we won't be able to add any other properties on the context but I don't think we will want that anyways?

@alexandernanberg

This comment has been minimized.

Member

alexandernanberg commented Aug 9, 2018

~~~Okay so I've hit a problem that I don't really get why it's occurring. The BaseStyledComponent doesn't seem to get any static properties, like attrs, target and so on. They will be undefined in the render method which causes all sorts of issues.~~~

Edit: Wow I actually used this.target instead of this.constructor.target 🤦‍♂️

@alexandernanberg

This comment has been minimized.

Member

alexandernanberg commented Aug 9, 2018

Tests are passing 🎉

Would love to get some second opinions and feedback on this now @kitten @mxstbr @probablyup

@probablyup

This comment has been minimized.

Contributor

probablyup commented Aug 9, 2018

Nice! Ok so the last thing I want to do here is some performance analysis.

Specifically making sure we aren't regressing against current master. Could you rig up a test using ThemeProvider in the benchmark suite (it's in this repo under benchmarks.)

@probablyup

This comment has been minimized.

Contributor

probablyup commented Aug 9, 2018

Let's also switch this branch to be pointing at "develop" which is the 4.0 development branch I just made.

@probablyup probablyup referenced this pull request Aug 9, 2018

Closed

4.0 Roadmap #1694

18 of 19 tasks complete

@alexandernanberg alexandernanberg changed the base branch from master to develop Aug 9, 2018

@alexandernanberg alexandernanberg changed the title from [WIP] Use new React context API to Use new context API Aug 10, 2018

@alexandernanberg

This comment has been minimized.

Member

alexandernanberg commented Aug 10, 2018

Okay so I’m not sure that I interpret the benchmark results correctly, is a high or low number better? First thought it was high=better but I just noticed the ms suffix which made me uncertain

Anything you want me to fix before this can get merged?

@probablyup

This comment has been minimized.

Contributor

probablyup commented Aug 11, 2018

Lower ms is better. I think @kitten is right though, there are a bunch of other changes we're going to make in this branch so perf analysis might be premature.

@@ -150,7 +148,7 @@
"typescript": "^2.4.1"
},
"peerDependencies": {
"react": ">= 0.14.0 < 17.0.0-0"
"react": ">= 16.3.0 < 17.0.0-0"

This comment has been minimized.

@probablyup

probablyup Aug 11, 2018

Contributor

Let's just make this >= 16.3 for simplicity.

return React.createElement(Component, props)
return (
<ThemeConsumer>
{(theme?: Theme) => {

This comment has been minimized.

@probablyup

probablyup Aug 11, 2018

Contributor

Let's cache the function on the class so it's not getting recreated per render.

return (
<StyleSheetConsumer>
{(styleSheet?: StyleSheet) => (

This comment has been minimized.

@probablyup

probablyup Aug 11, 2018

Contributor

Same here, let's cache it on the class.

return createElement(target, propsForElement, children)
return (
<ThemeConsumer>
{(theme?: Theme) => {

This comment has been minimized.

@probablyup

probablyup Aug 11, 2018

Contributor

And here

constructor(props: Props) {
super(props)
this.getContext = memoize(this.getContext.bind(this))

This comment has been minimized.

@probablyup

probablyup Aug 11, 2018

Contributor

Don't need to do constructor if you use this pattern on the class itself:

class ThemeProvider extends Component<Props> {
  getContext = memoize(
    (theme: (outerTheme: ?Theme) => void, outerTheme?: Theme) => this.getTheme(theme, outerTheme)
  )
}

This comment has been minimized.

@alexandernanberg

alexandernanberg Aug 11, 2018

Member

Yeah I actually wrote it similar to that first, but refactored after reading this thread. I was worried about booth perf and that the output isn't the same. Example here.

However if you feel that that's not a problem I'll happily change

This comment has been minimized.

@probablyup

probablyup Aug 11, 2018

Contributor

You can leave it though, I'll experiment with it both ways later

@probablyup

This comment has been minimized.

Contributor

probablyup commented Aug 11, 2018

This is looking great! Let's do this final cleanup pass and get it in!

@probablyup probablyup added the 4.0 label Aug 11, 2018

@alexandernanberg

This comment has been minimized.

Member

alexandernanberg commented Aug 11, 2018

Cool, in that case this PR actually outperforms the master branch, correct?

So I tested to use class properties instead of binding the function in the constructor and it performed worse :/ Feel free to try this yourself as well, in case I did something wrong.

Also tested to break out the inlined functions inside the render method, and that didn't seem to make any notable difference other than we run into the same problem described above.

It might be easier to get this in the develop branch and then iterate on these things?

@probablyup

This comment has been minimized.

Contributor

probablyup commented Aug 11, 2018

@alexandernanberg Yeah that's possibly true! We did make a lot of changes.

@@ -2,117 +2,93 @@
/* eslint-disable react/no-multi-comp */
import React from 'react'
import { shallow, render, mount } from 'enzyme'
import ThemeProvider, { CHANNEL_NEXT, CONTEXT_CHANNEL_SHAPE } from '../ThemeProvider'
import ThemeProvider, { ThemeConsumer } from '../ThemeProvider'

This comment has been minimized.

@probablyup

probablyup Aug 11, 2018

Contributor

Maybe instead of ThemeConsumer we should test this with the withTheme HOC and normal styled constructors instead. That way we are ensuring the external API doesn't change.

This comment has been minimized.

@alexandernanberg

alexandernanberg Aug 11, 2018

Member

Okay so would this be OK or do we want to use expectCSSMatches function as it's done in theme.test.js?

  it('should merge its theme with an outer theme', () => {
    const outerTheme = { main: 'black' }
    const innerTheme = { secondary: 'black' }

    const childrenSpy = jest.fn()
    const MyDiv = ({ theme }) => {
      childrenSpy(theme)
      return null
    }
    const MyDivWithTheme = withTheme(MyDiv)

    render(
      <ThemeProvider theme={outerTheme}>
        <ThemeProvider theme={innerTheme}>
          <MyDivWithTheme />
        </ThemeProvider>
      </ThemeProvider>
    )

    expect(childrenSpy).toHaveBeenCalledTimes(1)
    expect(childrenSpy).toHaveBeenCalledWith({
      ...outerTheme,
      ...innerTheme,
    })
  })
@probablyup

This comment has been minimized.

Contributor

probablyup commented Aug 11, 2018

I'm happy with this once the last testing point is resolved. Excellent work 👊

@alexandernanberg

This comment has been minimized.

Member

alexandernanberg commented Aug 11, 2018

Superb! Thank you 😄

@probablyup

This comment has been minimized.

Contributor

probablyup commented Aug 12, 2018

It's going in!

@probablyup probablyup merged commit f242f9a into styled-components:develop Aug 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mxstbr

This comment has been minimized.

Member

mxstbr commented Aug 12, 2018

Thank you so much for helping us improve styled-components! Based on our Community Guidelines every person that has a PR of any kind merged is offered an invitation to the styled-components organization—that is you right now!

Should you accept, you'll get write access to the main repository. (and a fancy styled-components logo on your GitHub profile!) You'll be able to label and close issues, merge pull requests, create new branches, etc. We want you to help us out with those things, so don't be scared to do them! Make sure to read our contribution and community guidelines, which explains all of this in more detail. Of course, if you have any questions just let me know!

@alexandernanberg alexandernanberg deleted the alexandernanberg:migrate-to-new-context-api branch Aug 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment