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

Allow instances of Emotion #430

Closed
jstcki opened this issue Oct 24, 2017 · 16 comments · Fixed by #464
Closed

Allow instances of Emotion #430

jstcki opened this issue Oct 24, 2017 · 16 comments · Fixed by #464

Comments

@jstcki
Copy link

jstcki commented Oct 24, 2017

  • emotion version: 8
  • react version: n/a

I'm currently evaluating different CSS-in-JS solutions and one thing that seems missing from most is a mode where you create an instance of the sheet/registry and manage styles using this instance instead of relying on a global singleton.

Why do I need this? Aside from "global shared mutable state is bad" etc. I maintain Catalog, a style guide builder that can render components from your codebase (similar to Storybook). Catalog itself is a React app and currently uses Radium which I want to replace for several reasons. Because users can include any code, I need to make sure that the replacement styling library doesn't conflict with anything else, including other versions of the same library.

Something like this could work, probably even without breaking existing API:

import Emotion from 'emotion/class';

const emo = new Emotion({key: 'my-special-key'});

emo.css`
  /* ... css etc */
`

The Emotion instance basically would expose the same methods like the emotion/index module does.

Alternatively, css et al. could take the instance as an additional argument.

The key option has two purposes:

  • Identify the style tags with the instance key (<style data-emotion="my-key"> instead of <style data-emotion>)
  • Prefix the generated classes with this key instead of generic css- (although this could be a distinct option)

Other aspects:

  • An instance could have a destroy method which would clean up styles managed by it.
  • Extracting styles for SSR would be safer (see extractCritical does not seem to work when npm linking #349)
  • Setting options (e.g. for vendor prefixing) for a global singleton is weird (must set globals before the singleton is created) but simple with instances

For reference, styletron and fela already work like this.

@tkh44
Copy link
Member

tkh44 commented Oct 25, 2017

I like this idea on the surface, but I really need to think about it. I'm also curious what @mitchellhamilton thinks.

@hanford
Copy link

hanford commented Oct 30, 2017

+1

This is one of my bigger frustrations with various different css-in-js solutions. I'm using lerna and am building out a large react component library where each component has it's own build step and as a result I need different instances to play nice.

This bit me pretty hard with styled-components 2.x (weirdly it works fine w/ 1.2.X so I've downgraded to that).. I've also ran into this exact issue with vercel/styled-jsx#309

anyways, I'd like to switch to emotion as it seems superior to styled-components, and I'm really digging a lot of the newer additions like facepaint.

@tkh44
Copy link
Member

tkh44 commented Oct 30, 2017

@hanford Would it work if you created a sheet package in your lerna packages that was required but the other packages?

@hanford
Copy link

hanford commented Oct 31, 2017

Potentially, though decoupling styles from the given components is obviously not ideal

@tkh44
Copy link
Member

tkh44 commented Nov 1, 2017

What I mean is creating a package in your packages that imports emotion. It would then export that instance of emotion tied to a sheet. You could then import this instance of emotion in your individual components.

@hanford
Copy link

hanford commented Nov 3, 2017

@tkh44 🤔 that's an interesting idea, I'll give it a try in a few days!

@loklaan
Copy link

loklaan commented Nov 8, 2017

Sorry to appear out of nowhere 🏃💨

I've been looking for this feature in oss css-in-js libs for awhile, and would be happy to help out any way I can!

I anticipate that exposing this would require the babel plugin to accept further configuration. It could, for example, be given a set of alternative css signatures to look for? This becomes more important when multiple output static css files are expected.

My reason for caring is in the collapsable at the bottom of this comment. It could be a broad-enough problem to warrant exposing an Emotion class & adding extra functionality to the babel-plugin.

Thoughts? I'm keen to jump on this if you're willing to accept it in to emotion! :)

Case: Alternative styling systems

Here is a strong case for several instances of sheet/emotion in a page (and from the babel plugin, several extracted css files); alternative styling systems. They might need extra functionality to work-around CSS ordering issues.

I've been using a styling system for React apps that dictates how styles are applied to elements in components. eg. <div {...styles('container')} />

When called, it sources styling primitives from three potential locations:

  1. Default styles, which are usually found in the component's module file
  2. Local overrides, which are provided at usage-time of the component
  3. App-wide* overrides, which can be found being used by a top level theme provider component

This system ends up working really well for component re-use across differently branded apps, because one brand may need to drastically change some internal styling of a component; the dev didn't have to anticipate every styling case, and instead accommodates any kind of styling change!

That's the preamble done.

The problems that occur in Browsers, with this alternative styling system, are to do with CSS ordering.

For example, you'd expect your "App-wide" overrides to override defaults, right?

You should.

But depending on how you've structured your app & it's dependency graph, a component's "Default styles" might be given to your css-in-js lib after your App-wide overriding styles... This results in the default styles taking precedence over it's intended overrides, which breaks everything.

This class of problem can be solved b asking the css-in-js library, like emotion, to put the styles from each "location" into it's own <style> elem, or seperate css file, and order them in the page's head appropriately.

@tkh44
Copy link
Member

tkh44 commented Nov 8, 2017

But depending on how you've structured your app & it's dependency graph, a component's "Default styles" might be given to your css-in-js lib after your App-wide overriding styles... This results in the default styles taking precedence over it's intended overrides, which breaks everything.

This is not true with emotion. If you browse around the tests and look at the snapshots you'll see that we compose styles from their raw style strings and not with class names. (this applies to emotion generated class names only)

@loklaan
Copy link

loklaan commented Nov 8, 2017

Ah sorry, I wasn't too clear on explaining this ordering problem. (btw emotion's composition features are great!)

This particular styling system is platform agnostic, since we need it to work with React Native components & apps. It needs to "own" the final composition of styling primitives sourced from defaults or overrides, which would be classname strings and style objects.

For the web, each of these sources for our styling system could be classnames generated by emotion.

For example, say we have a Button component, and it's using this alternative styling system. It's given default styles generated with emotion via some styled HoC.

import { css } from 'emotion'

@styled({ container: css`color: blue;` })
class Button extends Component {
  render () {
    return <div {...this.props.styles('container')} ></div>
  }
}

We've generated a classname for the default styles of Button.

Elsewhere in the App, we could override them like this.

// App overrides
<StylesProvider components={{ 'Button': { container: css`color: green;` } }} />

// Local overrides
<Button styles={{ container: css`color: red;` }} />

Local overrides are intended to take precedence in this system. Our Button should get a color of red. But it actually just depends on where the class sits in CSS on the page.. Because CSS.

Problem goes away when we're not using CSS or inline styles, but since we want to take advantage of everything CSS has to give, we're left fighting CSS injection ordering issues.

We currently get around those issues by being very careful with our app's call order of our components vs app style overrides.

We also avoid memoization/insertions optimisation in emotion (and glamor) because the same string of CSS could be used later in the application's call order, but with the intention of using a newly inserted high (bottom) ordered class name. Our bundle size is a bit bigger.

This might all seem really specific right now, but it definitely relates to this github issue! Really keen to help out, given how much I have to gain from it.

@emmatown emmatown mentioned this issue Nov 15, 2017
3 tasks
@emmatown
Copy link
Member

emmatown commented Dec 6, 2017

I know it's been a while since this has been discussed but I'm getting close to finishing #464 and I want to see what people think.

@loklaan Could you explain how emotion's composition doesn't solve your issue? If you use cx, you can merge your classes like so and the styles will be ordered with defaultClassName being the least specific, appClassName being more specific and localClassName being the most specific

const newClassName = cx(defaultClassName, appClassName, localClassName)

@hanford Am I correct in assuming from slack that your issue is solved?

@herrstucki Could you look at #464 and see if it solves your issue?(you might want to look at the create-emotion README) It uses a factory instead of a class since we use this in certain parts of emotion and in babel-plugin-emotion we assume that emotion exports named exports. It doesn't have a key option like you mentioned, could you explain why you think it's necessary?

@jstcki
Copy link
Author

jstcki commented Dec 6, 2017

@mitchellhamilton wow, nice! I don't have much time to dig in right now but from reading the README, I have a couple of questions:

  1. I'm not sure I understand from the description what's going on in https://github.com/emotion-js/emotion/blob/instances/packages/create-emotion/README.md#context. Why does the context object need to be a property on the global or window object or global|window itself? context.__MY_EMOTION_INSTANCE__ is not shared between multiple instances, is it (it's in the description but not completely clear IMHO)?

Wouldn't this also work:

let myContext = {}; // not sure how this is different to window.__MY_CONTEXT__
export const {...} = createEmotion(myContext);
  1. I assume an instance created with createEmotion "just works" alongside a regular emotion singleton?

  2. The notion of "destroying" or cleaning up an instance's style tags is currently not there, right?

  3. The key option as I described above would allow to identify style tags and class names generated by a particular instance. This would be nice to identify things controlled by different instances in the dev tools and potentially target them with other scripts (e.g. to clean up). E.g.

<!-- head -->
<style data-emotion></style> <!-- singleton emotion -->
<style data-emotion="my-key"></style> <!-- my instance -->

<!-- body -->
<div class="my-key-12345"> <!-- my instance -->
  <div class="css-23456">...</div> <!-- singleton emotion -->
</div>

@emmatown
Copy link
Member

emmatown commented Dec 6, 2017

  1. I'm not sure I understand from the description what's going on in https://github.com/emotion-js/emotion/blob/instances/packages/create-emotion/README.md#context. Why does the context object need to be a property on the global or window object or global|window itself? context.__MY_EMOTION_INSTANCE__ is not shared between multiple instances, is it (it's in the description but not completely clear IMHO)?

I definitely need to change the explanation but anyway the context isn't shared between multiple explicit create-emotion instances. The reason for the global is if there is multiple implicit instances(the code creating the instance is called multiple times, it's in a bundle twice or something else like that), this is what causes the problem in #349 since emotion is in two different node_modules and emotion from both node_modules are used and they can't access styles from each other. I'm thinking about only using a global for SSR and not using window since the only time it would matter in the browser would be when the instance is in multiple bundles and that is generally very bad anyway. The example you showed would work, it would just cause the problems described in #349.

  1. I assume an instance created with createEmotion "just works" alongside a regular emotion singleton?

Yep. It works totally fine, regular emotion is just an instance created with create-emotion

  1. The notion of "destroying" or cleaning up an instance's style tags is currently not there, right?

It's not documented at all but sheet.flush() will remove the style tags. (this is different to flush which is also exported by emotion but will flush the sheet, clear the caches and reinject the sheet) This is even in the current version of emotion. We'll likely document it and possibly expose it more publicly.

  1. The key option as I described above would allow to identify style tags and class names generated by a particular instance. This would be nice to identify things controlled by different instances in the dev tools and potentially target them with other scripts (e.g. to clean up). E.g.

I have mixed feeling about this. I can see how it would be useful to identify the classes but you can get the tags associated with the instance from sheet.tags. Doing this would also mean we would have to fork jest-glamor-react for emotion(which we've been thinking about anyway for other reasons) since it would have to support more than css- classes. Do you have any other use cases for this or is it mainly seeing it in dev tools?

@jstcki
Copy link
Author

jstcki commented Dec 6, 2017

Thanks for the explanation (and the hard work you put into this!). I haven't tested it yet for my use case but I'd be glad to do so if that helps.

My – admittedly quite esoteric – purpose for keying style tags is to clone all style tags execpt my own app's into a dynamically created iframe (to emulate responsive views of third-party components). But having access to sheet.tags seems already perfect for this purpose.

The only other (also esoteric?) reason for instance-specific class names that comes to mind are clashes where the name's the same but the actual style's different. Admittedly, the chance is small but may become an issue when combining different versions or libraries which also use the css- prefix (like glamor?).

@stephenjwatkins
Copy link

I can't tell with certainty if this was addressed above (although it kind of sounds like it...), but I have a use case where I am generating an iframe using react-frame-component and need to be able to control what window's head styles are injected into. Does the work done in #464 allow for this? Would it be a matter of providing the iframe's window as the context, instead of the parent's window?

@emmatown
Copy link
Member

emmatown commented Dec 7, 2017

@stephenjwatkins The context is purely used as a way to store caches. It does not control where styles are inserted. I think we'll need an option like tag or container that specifies the DOM node to put the style tags in.

@herrstucki I thought about clashes and initially dismissed it for various reasons but thinking about it more it could definitely be an issue because the prefix and stylisOptions options run after hashing and they can change the styles so I think we might need a key option.

@stephenjwatkins
Copy link

stephenjwatkins commented Dec 7, 2017

@mitchellhamilton thanks for the clarification. An option for the DOM node would be wonderful for my use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants
@jstcki @tkh44 @stephenjwatkins @loklaan @hanford @emmatown and others