Upgrade guppy to styled components v4 #269

rfbotto · 2018-09-29T16:30:23Z

Related Issue:
#213

Summary:

Replace injectGlobal by the new createGlobalStyle
Replace innerRef for ref
- This also made me have to use React.forwardRef in the TextInput component in order to be able to forward the ref through the wrapper. This lead to a flow error as the flow types are not up to date with React > 16.3, hence the addition of the $FlowFixMe annotation.
- This also made me have to declare the respective ref variables as maybe instance types (https://flow.org/en/docs/react/refs/) which in result led to some extra conditional checking before their usage.
Replace .extend for styled()
Update the Heading component to use the new as prop, instead of withComponent

joshwcomeau

Hi @rfbotto!

Sorry it took me a couple days to get to this - I was at React Boston this weekend.

Overall it looks pretty good! I have a couple inline comments, mostly just small nitpicky things.

I have 1 big concern, though, about the performance with the new forwardRef.

Here's the profiler, comparing selecting a different project:

Before:

After:

(I also only tested in development mode. Maybe this is a non-issue in production... although I think dev mode performance is still important, so that Guppy is pleasant to work on and we can get a roughly-accurate sense of how our changes affect performance)

It looks like the issue is forwardRef. For some reason it does a whole bunch of work.

I'm wondering if this might be fixed when we merge #265. That component already has some issues and I wonder if it's making this a lot worse. So I think I'd like to leave this PR open while we land that PR, and then we can check to see if that fixes it?

joshwcomeau · 2018-10-01T13:35:56Z

.storybook/config.js

+  <>
+    <GlobalStyles />
+    <Wrapper>{storyFn()}</Wrapper>
+  </>


Nit: We've been using Fragment instead of <> in this project:

import React, { Fragment } from 'react'; <Fragment> <GlobalStyles /> <Wrapper>{storyFn()}</Wrapper> </Fragment>

It does exactly the same thing, but it's a bit more google-able, so for folks who haven't seen it before, it's not as mysterious/magical.

joshwcomeau · 2018-10-01T13:38:18Z

package.json

@@ -132,7 +132,7 @@
    "sass-loader": "7.0.0",
    "slug": "0.9.1",
    "style-loader": "0.19.1",
-    "styled-components": "3.2.6",
+    "styled-components": "^4.0.0-beta.9",


Nit: We tend to prefer exact versions, without the ^.

You can specify this when installing by doing yarn add -DE dependency@version. -D for devDependencies, -E- for exact version.

joshwcomeau · 2018-10-01T13:40:06Z

src/components/AddDependencySearchBox/AddDependencySearchBox.js

@@ -33,7 +34,7 @@ const Wrapper = styled.div`
  width: 100%;
 `;

-injectGlobal`
+const SearchBoxGlobalStyle = createGlobalStyle`


this is so much nicer :D

joshwcomeau · 2018-10-01T13:42:18Z

src/components/CreateNewProjectWizard/ProjectName.js

-    this.node.focus();
+    if (this.node) {
+      this.node.focus();
+    }


hah, nice. Digging that switching to ref means that Flow now catches more issues

joshwcomeau · 2018-10-01T13:48:48Z

src/components/WhimsicalInstaller/WhimsicalInstaller.js

@@ -68,10 +68,12 @@ class WhimsicalInstaller extends PureComponent<Props, State> {

  toggleRunning = () => {
    if (this.props.isRunning) {
-      this.wrapperBoundingBox = this.wrapperNode.getBoundingClientRect();
+      if (this.wrapperNode) {


I think we can roll this into the previous condition:

if (this.props.isRunning && this.wrapperNode) {

I like reducing nested conditionals, when possible.

joshwcomeau · 2018-10-01T13:51:32Z

src/global-styles.js

@@ -1,10 +1,10 @@
-import { injectGlobal } from 'styled-components';
+import { createGlobalStyle } from 'styled-components';


Hm, so with the new API, I think this should become a component, living in src/components/GlobalStyles/GlobalStyles.js (with an index.js that links to it, see existing components for examples).

rfbotto · 2018-10-02T17:01:42Z

Hey @joshwcomeau!

I hope you had a great time at React Boston, looking forward to seeing your whimsy talk! :)

I updated the PR with the changes you request. In the meantime, while #265 doesn't get merged, i will also try to investigate into the reason for the performance constraint with the usage of forwardRef in the TextInput.

joshwcomeau

This looks really good!! Thanks for making all my requested changes :D

I checked out this branch locally and rebased master. I fixed the conflicts, but because I did it with a rebase, I didn't want to push directly to your branch (since you'd need to hard-reset it).

I pushed my conflict-fix to improvement/213-upgrade-styled-components-to-v4-rebased, if you wanna test things out there. You can also tackle the merge yourself (for the conflicts, just take what's in master, since I no longer need the ref in CircularOutline.

The performance is much better now, but still imperfect. Here's a side-by-side (The one on the left is your branch, the one on the right is the new master):

I dug into this a little bit, but I'm confused why React is re-rendering so much. If you wanted to do some perf exploration, that'd be awesome, but I'm also happy to look at it myself as well :)

This PR is essentially approved, but I'd just love to get to the bottom of this before we merge it. All your changes are good, but it appears the new version of styled-components has exposed a pre-existing problem in our codebase, and we should fix that first 👍

joshwcomeau · 2018-10-03T12:43:57Z

.storybook/config.js

+    <GlobalStyles />
+    <Wrapper>{storyFn()}</Wrapper>
+  </Fragment>
+);


Good catch!

joshwcomeau · 2018-10-03T12:45:54Z

src/components/GlobalStyles/GlobalStyles.js

+  }
+`;
+
+export default GlobalStyles;


Looks great 👍

joshwcomeau · 2018-10-03T13:09:16Z

Oh! Something really interesting: I was assuming the problem was with <CircularOutline>, since the only visible animation is the outline around the project icons in the sidebar... but when I disable that animation, the performance problem still exists in the profiling tools.

So, there's clearly a <Spring> somewhere re-rendering a bunch when it doesn't have to 🤔

Will continue looking later, but feel free to do some digging if it interests you :D

superhawk610 · 2019-03-28T22:51:44Z

I'm afraid master has diverged too substantially to be able to reasonably recover this PR. I'm going to open a new PR and do my best to cherry pick all novel contributions from this. @rfbotto please feel free to critique/add to the new PR - I will link it here when it's open.

Migrate guppy to styled components v4.

90889a0

rfbotto self-assigned this Sep 29, 2018

joshwcomeau reviewed Oct 1, 2018

View reviewed changes

rfbotto added 2 commits October 2, 2018 18:57

Integrate PR feedback.

c2083a6

Remove global-styles.js

7922b26

joshwcomeau reviewed Oct 3, 2018

View reviewed changes

Merge changes to circular outline.

cd7836e

melanieseltzer mentioned this pull request Jan 18, 2019

Gatsby starter #335

Merged

18 tasks

superhawk610 closed this Mar 28, 2019

superhawk610 added a commit that referenced this pull request Mar 28, 2019

migrate from #269

a5b0670

superhawk610 mentioned this pull request Mar 28, 2019

Upgrade to styled-components v4.2.0 #374

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade guppy to styled components v4 #269

Upgrade guppy to styled components v4 #269

rfbotto commented Sep 29, 2018

joshwcomeau left a comment

joshwcomeau Oct 1, 2018

joshwcomeau Oct 1, 2018

joshwcomeau Oct 1, 2018

joshwcomeau Oct 1, 2018

joshwcomeau Oct 1, 2018

joshwcomeau Oct 1, 2018

rfbotto commented Oct 2, 2018

joshwcomeau left a comment

joshwcomeau Oct 3, 2018

joshwcomeau Oct 3, 2018

joshwcomeau commented Oct 3, 2018

superhawk610 commented Mar 28, 2019 •

edited

Loading

		@@ -1,10 +1,10 @@
		import { injectGlobal } from 'styled-components';
		import { createGlobalStyle } from 'styled-components';

Upgrade guppy to styled components v4 #269

Upgrade guppy to styled components v4 #269

Conversation

rfbotto commented Sep 29, 2018

joshwcomeau 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

rfbotto commented Oct 2, 2018

joshwcomeau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshwcomeau commented Oct 3, 2018

superhawk610 commented Mar 28, 2019 • edited Loading

superhawk610 commented Mar 28, 2019 •

edited

Loading