Adding useImportant config

[JedWatson]

This is an alternative implementation to #41 based on our discussion there. Closes #25

Given the concepts that:

• The use-case for adding !important to all generated styles is having a project with a legacy stylesheet that could clash with Aphrodite's classes
• Aphrodite can be used by components / libraries that should have no knowledge or opinion of whether !important is applied to the style definitions
• We want a consistent API for opting in or out of !important for all styles generated by Aphrodite in a project (and that this shouldn't be left to component authors to expose)

I've implemented config that can be set on the aphrodite package itself, and a single option that controls whether to use important or not.

So now to turn on !important, you would do the following once in your project (probably alongside the app entry point):

import { css, setConfig, StyleSheet } from 'aphrodite';
setConfig('useImportant', true);

This way, the setting will be applied consistently throughout the project regardless of the components being used, which I think is the best way to have this work.

The only downside I can see is that if someone uses a component styled by Aphrodite (without using Aphrodite in their project directly), and they needed to turn the useImportant setting on, they'd need to depend on aphrodite and include this somewhere in their project:

require('aphrodite').setConfig('useImportant', true);

Given that the alternative is for components to individually expose an API for this, I think it's a reasonable trade-off. (come to think of it, you could expose that as an API on a component package anyway, although I'm not sure it's a good idea...)

I've updated the existing unit tests, and added tests for the new functionality. I'm happy to update the Readme with documentation as well, but wanted to submit this for review first.

Overall I've tested this in my own projects and am really happy with how it works, two thoughts on the new config concept:

• It seems like a little bit of overkill to add a config API when there's only one option, but it seemed cleaner than exposing something like aphrodite.useImportant(true) which wouldn't scale well if more options are added in the future.
• You could easily support syntax like aphrodite.setConfig({ useImportant: true }) but the way I'd do that requires the Object.assign polyfill in babel which isn't configured, so I left that out for simplicity.

Finally, this implementation could behave unpredictably if more than one version of Aphrodite is included in a project... Given that's an edge case that React also has issues with, I think it's a reasonable trade-off.

Hope you like it!

[jlfwong]

Thanks for the thorough PR description and tests!

If I'm reading this right, this would be a major version change, because it changes the default to not use !important, right?

I haven't read through the code yet, but the idea of a global configuration like this is unsettling to me, especially because it means that import order becomes important.

If a component author distributes a component that does setConfig('useImportant', true);, then somebody if you want to override that, it's up to them to ensure that their setConfig comes after the one set in component they import. I guess the argument you'd like to lay down is to tell component authors "don't use setConfig"?

It also means that doing a combination of !important and not using !important! becomes really awkward. What would you do? Toggle between the settings via several run-time calls to setConfig?

Overall, I'm still much more comfortable with the aphrodite/without-bang-important solution. I recognize that puts the onus of whether or not of deciding whether or not the components have !important, but that's been the case with people distributing components forever so far anyway, just with a different default.

[JedWatson]

@jlfwong correct, as written this would be a major / breaking API change, I was hoping this would become 1.0.0 as with configurable important behaviour, Aphrodite seems quite stable in terms of features and API.

re: your concerns about global configuration, absolutely - component authors should never set useImportant.

To explain this, as the author of react-select I don't know whether it's going to be used in an environment where existing stylesheets might interfere with the component styles. So I can't make that call. If you use it on Khan Academy, you'll want !important appended to the style definitions. When someone else uses it, they may want the opposite.

So ideally I can use use Aphrodite to colocate my styles with my component, and add a link in my readme to Aphrodite's docs explaining how to configure the !important setting if they get style conflicts.

The alternative that's been suggested is each component author comes up with their own custom API and implementation to allow users to set whether the component uses aphrodite/without-bang-important or aphrodite/with-bang-important internally, which doesn't scale well.

re: a combination of !important and not using !important, I can't imagine a use-case for this? There are either conflicting stylesheets in the website (Khan's use-case) or not (greenfield project). You're right that if some components in a project need !important and others didn't, this approach is not a good way to handle it.

Come to think of it, we could make it something you can set once and once only per runtime, and warn users if there could be conflicts (the way React warns you about certain things).

If there's a valid use-case for some components forcing !imporant on their styles, you could simply include !important in the style definitions that needed it. That's how people have been doing it forever 😉

Seriously though, I think a better API for that than either changing Aphrodite's config or importing aphrodite/with-bang-important is to wrap the StyleSheet.create argument in a utility function. I'm happy to add something like this to the PR if it'll make you more comfortable merging it. Usage would be:

import React, { Component } from 'react';
import { StyleSheet, setConfig, importantify, css } from 'aphrodite';

class App extends Component {
  render() {
    return <div className={css(styles.red)}>This is red.</div>;
  }
}

const styles = StyleSheet.create(importantify({
  red: {
    backgroundColor: 'red'
  },
}));

Behind the scenes, importantify would rewrite the styles definition to be:

{
  red: {
    backgroundColor: 'red !important'
  },
}

The benefit to this approach is that the use of !important is determined in the StyleSheet creation, not at runtime when the styles are applied. And it's still a nice API because you don't have to add ! important throughout your style definitions, but the fact that it's being applied is explicit.

Finally, I want to call out vercel/hyper#99, which is a really interesting case for when !important is harmful and being able to override StyleSheet styles with the inline style prop is important. Unlike the use-cases discussed so far (web project with legacy stylesheets, open source components like react-select) this is a "native" application exposing an extension and themeing API that Aphrodite's blanket addition of !important is messing with.

Anybody building extension components for that project should simply need to understand import {StyleSheet, css} from 'aphrodite' for styling, not the distinction between aphrodite/with-bang-important and aphrodite/without-bang-important. Leaving !important out should be the project author's call.

[jeffbski]

I believe it's really like to have !important be configurable as well. This is really necessary for allowing this to play well with external stylesheets.

Aphrodite has so much going for it, but this is a huge thorn that needs to be addressed or it will drive users to other projects.

[jlfwong]

I really appreciate the thorough discussion here, and don't at all want to disregard the effort in both the implementation here or the thought put into your comments, but I really don't think this is the right path.

I'm really not sold on making this a global config. IMO, whether or not your component is using aphrodite should be an implementation detail that is not exposed to consumers.

The ability to magically control whether or not all of a third party component's styles have !important appended or not is not a thing that people have needed to do in the past with third party components, and I don't see it suddenly becoming a need that everyone has. If you're using third party components, presumably some of them will not be written with aphrodite, and that should be okay.

Consider this scenario:

• Your component is built on aphrodite with this PR merged.
• Some consumers of your library then do aphrodite.setConfig('useImportant', true);
• You now want to remove aphrodite as a dependency of your component. You now have absolutely no way of doing this without introducing a nasty breaking change AND providing a mechanism for setting it yourself. It seems likely to me that aphrodite will eventually be replaced with something more core to React, so the lack of migration path off of aphrodite sounds pretty terrible.

This PR is, IMO, trying to solve 3 problems at once, one of which I'm not convinced should be solved.

First, it's trying to make it possible to set up styles without !important. I'm convinced this is a good idea.

Second, it's trying to make it default to use styles without !important. I'm lukewarm on this, but undecided.

Third, it's trying to make it possible for things using your npm-installed component to control the !important behaviour of your library without you, as component author, needing to expose an API for it. For the reasons stated above, I don't think this is a good idea.

I'm going to implement the aphrodite/without-bang-important solution, so we at least solve that first problem, release that as a minor version change, then try to catch up on the discussion about making it default.

[JedWatson]

This PR is, IMO, trying to solve 3 problems at once, one of which I'm not convinced should be solved.

Fair enough! I agree this is contentious and #104 is a nice non-breaking change that moves things forward. Adding that will work for projects nicely.

I've got real concerns about being able to use Aphrodite in components (OSS) as well though, and without some kind of design change here Aphrodite's potentially harmful in that use case. We've been porting KeystoneJS / Elemental UI to it for a week now and it's been troubling working through a project of that complexity.

Any chance we can get on a hangout or something to discuss rather than going back and forward here? I'd really like to figure out if Aphrodite's likely to move in a direction that works for scalable OSS or if I should be looking for a different thing.

(No judgement, not here to tell you what your project needs to be or how to do it, just want to figure some stuff out before we invest in a use-case you may not be interested in or may not want to compromise other concerns for)

[kentcdodds]

I'd love to be on that call if I can be so presumptuous to invite myself 😄 I think a synchronous conversation about this would be helpful.

[sophiebits]

I've got real concerns about being able to use Aphrodite in components (OSS) as well

If you want people to be able to override your styles, just use no-important in your component, right? That should match how you might distribute components without Aphrodite already.

[JedWatson]

@spicyj breaks usage w/ existing stylesheets, same reason the important default was added in the first place. I like the concept of some way to aggressively raise precedence, it's valid, I see that as a build-step-like thing (similar to adding plugins to your css loaders in Webpack)

i.e. using css-modules, your component does require('styles.css'). How that's packaged / transformed is up to the project config, not the component. Adding !important is a project specific transform, like using an auto-prefixed for the browsers your app supports is a project specific transform.

At the moment we're using LESS so people can modify the generated stylesheet pretty easily in the build step... we have no opinions about how the styles are actually added to the page, using Aphrodite changes that in a way that removes control from the component user.

I've written a lot at this point though, like @kentcdodds said a sync conversation would be really beneficial.