marionebl commented 6 years ago

Picks up work by @JamieDixon in #1416

originally motivated by #1333
newly motivated by #1491

ToDo

[x] Solve merge conflicts
[x] Add tests for new behaviour
[x] Add support for function interpolation
[x] Add support for themes
[x] ~~Add support for `keyframes``` interpolation~~

Example

import React from 'react';
import {render} from 'react-dom';
import {ThemeProvider, createGlobalStyle} from 'styled-components'

const GlobalStyle = createGlobalStyle`
  * {
    box-sizing:  border-box;
  }
  body {
    background: ${props => props.theme.background};
    font-family: ${props => props.fontFamily};
  }
`

const App = () => (
  <ThemeProvider theme={{background: 'palevioletred'}}>
     <GlobalStyle fontFamily="Consolas" />
  </ThemeProvider>
)

render(<App />, document.querySelector('[data-root]'))

Demo

Published versions

npm install @marionebl/styled-components@3.3.0

mxstbr-bot commented 6 years ago

	Warnings
:warning:	There are library changes, but not tests. That's OK as long as you're refactoring existing code

Generated by :no_entry_sign: dangerJS

Fer0x commented 6 years ago

But what about keyframes?

mxstbr commented 6 years ago

Good question @Fer0x. They have the same issue as injectGlobal: They live outside of the component tree, which makes things like #1324 hard to implement.

Opened #1496, let's discuss there.

marionebl commented 6 years ago

This one bugged be a bit in the previous PR too:

https://travis-ci.org/styled-components/styled-components/jobs/338113464#L624

Invariant Violation: StyleSheetManager.getChildContext(): key "__styled-components-stylesheet__" is not defined in childContextTypes.

For some reasone the import of CONTEXT_KEY seems to yield undefined in some tests?

https://github.com/styled-components/styled-components/blob/474946d58db59c342de8d6cf8dc715cde592daf7/src/models/StyleSheetManager.js#L4

marionebl commented 6 years ago

Went ahead and

implemented some tests for this
also started implementation of interpolation in createGlobalStyle strings

I am pretty sure this could be implemented way DRYer, but could not find the reusable bits and pieces for interpolation, theme handling etc. I expected.

Pointers to the places in the code based would be appreciated!

The failing test case about theme updates reflects the buggy WIP implementation.

I won't be able to work on this over the next two weeks, so please don't hesitate to pick this up.

kitten commented 6 years ago

I am pretty sure this could be implemented way DRYer, but could not find the reusable bits and pieces for interpolation, theme handling etc. I expected.

@marionebl I added some comments with the things that I've found. Unfortunately all this stuff wasn't extracted into a common place yet, even when it became apparent during a refactor of withTheme that it'll probably need to be extracted. But let's take a look at that in a separate PR

If you need help to use ComponentStyle, css, copying the theming stuff, etc, feel free to ping me on Twitter DMs if that helps you to get this done faster :+1:

kitten commented 6 years ago

@marionebl A StyleSheet#remove method will be added in #1514. It takes a componentId as an argument similarly to the removeComponent method you added as part of this PR

marionebl commented 6 years ago

@kitten @mxstbr Could you have another look?

marionebl commented 6 years ago

For people reviewing this I'd like to point out two parts:

src/models/GlobalStyle - introduced to avoid even higher complexity in ComponentStyle
src/hoc/withTheme.js#L48-L53 - disables the warning in withTheme about missing ThemeProvider if the decorated Component is found to be a styled-component. This enables the usage of withTheme in createGlobalStyle. Perhaps withTheme could be used in src/models/StyledComponent, too?

kitten commented 6 years ago

@marionebl I’ll try to find some time next week to review this. Generally GlobalStyle might be a good idea, but since it’ll share a lot of code with ComponentStyle I’m not sure whether we shouldn’t find a better abstraction for both. I’d be happy to take on this task in a separate branch though.

Regarding withTheme, I’d like to avoid using it for internal components. It’s true that we need to improve our code sharing here, but the logic that components apply to themes is generally meant to be shared through utilities as we’d like to avoid another level of components for StyledComponents

marionebl commented 6 years ago

[...] Generally GlobalStyle might be a good idea, but [...] I’m not sure whether we shouldn’t find a better abstraction for both. I’d be happy to take on this task in a separate branch though.

👍 for introducing a code sharing abstraction in a separate PR

Regarding withTheme, I’d like to avoid using it for internal components [...]

Understood. Would createGlobalStyle without theme support be an acceptable intermediary step? I imagine we could add theme support (with its apparent complexity) in a later PR.

kitten commented 6 years ago

@marionebl I'd be happy to add theme support and refactor a couple of things myself, if you don't mind :+1: It's been on my todo list either way for a while 😅

marionebl commented 6 years ago

I'd be happy to add theme support and refactor a couple of things myself, if you don't mind 👍 It's been on my todo list either way for a while 😅

Ok. Just trying to find a way to get global styles that honor StyleSheetManager.target 😅 asap

marionebl commented 6 years ago

That being said I'd like to take part in the refactorings if you provide a rough outline and illustrating pseudo code if applicable. 🙇

chenxsan commented 6 years ago

Sorry, didn't mean to toggle the checkbox when clicking on it :(

ermik commented 6 years ago

I am actively choking as I'm forced to write the spaghetti from #793 right now, can't wait till this is production-ready. Thank you very much for the hard work — I can't speak for the community, but I will: You're al our heroes (and by that, I obviously mean you guys are major visual assets@2x).

wmertens commented 6 years ago

@marionebl could you resolve the merge conflicts (very minor)

@kitten @mxstbr I think this is ready for prime time… Can you check?

marionebl commented 6 years ago

@wmertens Solved the conflicts. @kitten requested the internal withTheme usage to be removed in order to avoid another layer of components in the tree.

I held back with implementing this due to @kitten mentioning a pending refactoring for code sharing concerns around internal theme support.

shaunbent commented 6 years ago

Do we think this is going to be merged soon?

I am dependant on this in order to be able to inject global styles within the correct context of <StyleSheetManger target={someFrame} />.

mxstbr commented 6 years ago

@shaunbent not sure, honestly. We've all been busy, somebody just needs to:

Add a depreciation warning to .extend and injectGlobal, publish a v3 minor release with those warnings
Merge all the big API changes to a v4 branch and publish a @next release

If you have time I'll add you to the repo? :pray:

wmertens commented 6 years ago

@mxstbr hmm, I understand about adding the deprecation, but this PR doesn't break anything does it? So we can add the new API before v4?

mxstbr commented 6 years ago

So we can add the new API before v4?

Yeah sure if we want to increase our bundle size by a couple kB gzip for a feature nobody doesn't even know exists :wink: I understand where you're coming from, but at this point we might as well publish a v4 alpha instead of squeezing this into a v3 release.

wmertens commented 6 years ago

I don't think it's even a couple KB non-gzip?

It's just kind of weird to deprecate a feature but not provide the alternative, no? Then the deprecation should be in v4 and only removed in v5…

On Wed, May 9, 2018 at 1:14 PM Max Stoiber notifications@github.com wrote:

So we can add the new API before v4?

Yeah sure if we want to increase our bundle size by a couple kB gzip for a feature nobody doesn't even know exists 😉 I understand where you're coming from, but at this point we might as well publish a v4 alpha instead of squeezing this into a v3 release.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/styled-components/styled-components/pull/1493#issuecomment-387705745, or mute the thread https://github.com/notifications/unsubscribe-auth/AADWltn1Z62AE_5Ge890ZPPuHFQ0ilCBks5tws-WgaJpZM4R6Efp .

mxstbr commented 6 years ago

It's just kind of weird to deprecate a feature but not provide the alternative, no?

Yeah I guess that's true. 🤔 Good point. So we should add the warnings but also the new APIs.

kitten commented 6 years ago

@mxstbr as discussed previously I think we’re unfortunatelt forced to release a proper v3 with a deprecation and the new APIs :/ on the other hand, we can make sure that said release only contains this one change so people can hold off if they don’t want their bundle size to increase

JamieDixon commented 6 years ago

I'm super excited to see this getting towards a release! Amazing work @marionebl 🎉

quantizor commented 6 years ago

I'd prefer any changes outside the scope of this PR to be made in a separate PR. Thanks!

migueloller commented 6 years ago

@probablyup, I'm not sure if this is in reference to my comment. If it is, the comment is well within the scope of the PR. If it isn't then disregard this comment! 😄

migueloller commented 6 years ago

There seems to be an off-by-one bug in the stylesheet manager. Note, this only happens when process.env.NODE_ENV is production and a component created with createGlobalStyle is used.

Here's a screenshot of the error:

kitten commented 6 years ago

@migueloller It looks like I messed up here: https://github.com/styled-components/styled-components/blob/master/src/utils/insertRuleHelpers.js#L52

If we delete from b and want to delete 3 items. We'd want b - 2, b - 1 and b, but the stop index is b - size (3) and we compare it using >= so we delete one item too many. Can simply be fixed by changing the comparison to i > lowerBound instead, I believe.

migueloller commented 6 years ago

@kitten, awesome. Thanks for the quick response! Will open up a PR.

eav commented 6 years ago

I'm stuck in iframe/global styles, looking forward this feature!

marionebl commented 5 years ago

Peeplz! Thanks for all the great feedback and patience.

GlobalStyleComponent now works outside of a theme provider
GlobalStyleComponent now accepts children
Added a TODO comment about the withTheme usage not being here to stay
Readded a comment explaining STATIC_EXECUTION_CONTEXT
solved the merge conflicts
published the current state of the PR at @marionebl/styled-components
updated the demo at https://codesandbox.io/s/xow4x7p284

quantizor commented 5 years ago

@mxstbr what was the reasoning behind allowing this component to accept children? Looking at the demo, it's kind of confusing what scope the styles are being applied at. I think it'd make more sense as a standalone component.

marionebl commented 5 years ago

@mxstbr what was the reasoning behind allowing this component to accept children? Looking at the demo, it's kind of confusing what scope the styles are being applied at. I think it'd make more sense as a standalone component.

I followed @migueloller's reasoning. This is mainly about ergonomics - I felt being able to use it either way (instead of swallowing children like before) is the least surprising behavior for everyone.

quantizor commented 5 years ago

instead of swallowing children like before

I'd rather add a dev warning to not give it children and keep it as a meta component. The alternative is way more confusing IMO.

More context on reasoning: nesting in React implies direct ownership. If children are nested under "GlobalStyle" it's implied that "GlobalStyle"'s scope is restricted to that subtree. But that's not the case, since you can put arbitrary styles in there that can affect the whole document.

migueloller commented 5 years ago

@probablyup, I think that’s a fair point and I didn’t consider that when making the suggestion. I mostly focused on terseness, but it’s true that it might cause some confusion for devs.

Even in the example that I give, Normalize still applies to everything, not just its children.

marionebl commented 5 years ago

Ok circled back on the props.children change and added a development warning as @probablyup suggested.

quantizor commented 5 years ago

Nice! Ok this is looking good to me. Would like @kitten to do one last look-see to make sure we're not missing anything re: stylesheet lifecycles.

Could we also add an SSR test for this to make sure things are emitted properly?

marionebl commented 5 years ago

Added the relevant test as suggested by @probablyup

marionebl commented 5 years ago

@mxstbr @kitten @probablyup Any chance we could get this into a release this week? I'll be unavailable very soon (parental leave ❤️ ) and would like to see this through before then.

willwull commented 5 years ago

Tried out @marionebl/styled-components@3.3.2, but I'm still getting this error, production mode with create-react-app:

migueloller commented 5 years ago

@willwull, this should've been fixed by this.

@marionebl, I'm guessing you rebased v3.3.0?

willwull commented 5 years ago

@migueloller I tried it in another test project (using 3.3.2), but I can't seem to reproduce the error, it only occurs in the first project I tried it in. Why does it only happen in production mode? I'm trying to pinpoint if I did something strange, but it's a bit slow since I have to build the project each time 😅

willwull commented 5 years ago

@marionebl @migueloller

Update: seems this error only happens when trying to switch the theme for the ThemeProvider. Here is a minimal example, where clicking the button will crash the app:

package.json

{
  "name": "styled-components-test",
  "version": "0.1.0",
  "private": true,
  "dependencies": {
    "@marionebl/styled-components": "^3.3.2",
    "react": "^16.4.1",
    "react-dom": "^16.4.1",
    "react-scripts": "1.1.4"
  },
  "scripts": {
    "start": "react-scripts start",
    "build": "react-scripts build",
    "test": "react-scripts test --env=jsdom",
    "eject": "react-scripts eject"
  }
}

src/index.js

import React, { Component } from "react";
import ReactDOM from "react-dom";
import { createGlobalStyle, ThemeProvider } from "@marionebl/styled-components";

const GlobalStyle = createGlobalStyle`
  h1 {
    color: ${props => props.theme.color};
  }
`;

class App extends Component {
  state = {
    theme: {
      color: "red"
    }
  };

  toggleTheme = () => {
    this.setState({
      theme: {
        color: "blue"
      }
    });
  };

  render() {
    return (
      <ThemeProvider theme={this.state.theme}>
        <React.Fragment>
          <GlobalStyle />
          <div className="App">
            <h1 className="App-title">Welcome to React</h1>
            <button onClick={this.toggleTheme}>Click me</button>
          </div>
        </React.Fragment>
      </ThemeProvider>
    );
  }
}

ReactDOM.render(<App />, document.getElementById("root"));

Disclaimer: I'm extremely new to styled-components, so tell me if I'm doing something weird.

quantizor commented 5 years ago

Going to pick this up Monday or Tuesday to finish out any remaining items.

migueloller commented 5 years ago

@probablyup,

I can confirm that the deleteRule error that @willwull is mentioning is still there. This bug only happens in production.

In the process of trying to figure out what is wrong I came across a few inconsistencies between dev and prod. One of them is the fact that safeInsertRule is swallowing 2 errors in production caused by normalize from https://github.com/styled-components/polished. Could this be the cause for the deleteRule size and index mismatch?

Here's a screenshot of the errors logged to the console:

During debugging we commented out this line and it seemed to remove the issue, so it could be a problem with updateStyles making it be in a inconsistent state. Some of the deleteRule errors thrown come from componentWillRecieveProps as well, it's not all from componentWillUnmount.

Another issue we're having is duplication of styled from global style components. In the server we have a 2-render pass because of Apollo, this causes global styles to be applied twice. Then, once React hydrates in the client, the styles are applied once more, resulting in triple styles coming from global style components. It would seem that the IDs aren't stable, removing idempotency from these components. We're using the Babel plugin, too. This might be a different issue (style duplication), but I thought maybe the lack of idempotency might contribute to the issue at hand.

The way we ended up fixing the issue for now was by putting our global style components all the way outside of our app so that they're only ever rendered once. Before they used to be in our app wrapped by things like react-router's BrowserRouter and react-apollo's ApolloProvider. Whenever a route changed and caused a re-render, the deleteRule error would happen. This is very bizarre, given that the render method for global style components doesn't actually do anything, and shouldn't be the cause for side effects.

Thoughts?

quantizor commented 5 years ago

1807 supercedes this, closing (it's a cumulative PR)

styled-components / styled-components

Add component-based global styling API #1493

1807 supercedes this, closing (it's a cumulative PR)