Allow additional handlers for special selectors to be added.

[xymostech]

Summary: This adds the ability for users of Aphrodite to add extensions
which support other special selectors. I'm not sure if the interface
that is exposed is expressive enough to allow people to make all of the
changes that they might want to, but it would let people do the
selector(...) thing:

const regex = /^selector\((.*)\)$/;

function(selector, baseSelector, callback) => {
    const match = selector.match(regex);

    if (!match) {
        return null;
    }

    return callback(`${baseSelector} ${match[1]}`);
};

Maybe fixes #10

Test Plan:

• npm run test

@jlfwong @zgotsch @kentcdodds @montemishkin

[kentcdodds]

Could we call callback something else? When I see the word callback I think: "Call this thing when you're done." Maybe that's just me, but I think a more descriptive name would be appropriate.

Not sure what's best, but maybe: generateSubtreeStyles?

From a user perspective, it matters less in this code, but what we call it in the docs will be an important way to communicate what it's for.

[xymostech]

generateSubtreeStyles indeed sounds much better.

[kentcdodds]

This is an interesting idea. Unless I'm missing something, this would allow for a plugin ecosystem to be developed around aphrodite which would be neat.

[xymostech]

You aren't missing something, that's the idea!

[kentcdodds]

Though, at the same time, I'd like to figure out a way to ensure that the dependency a given set of styles has on such a plugin were more explicit... Maybe forcing that plugin be provided to any call into StyleSheet.create would be the way to do this. It could be seen as boilerplate, but it would lead to more maintainable systems.

[xymostech]

I thought about that too. The main problem I found with it is that these plugins are actually going to be used at css() time, and it sounds painful to require someone passing an extra argument into the css() call. :(

Also we currently cache and hash the styles based only on the classname, so if you tried to generate the same styles based on different plugins it wouldn't do anything?

[kentcdodds]

it sounds painful to require someone passing an extra argument into the css() call. :(

True, but what's less painful is if this can be curried in some fassion, like so:

const myCss = css(myCustomExtensions)

myCss(styles.bar, styles.foo)

Then people could pass myCss around and it'd still be explicit. It's slightly disappointing that we can't add this to StyleSheet.create because that's where the special selector stuff will be written. Is there any other way to implement this so that's where the extensions are defined?

Apologies that I haven't been helpful with actual code in aphrodite. I haven't had the chance to dive into the code much. Hopefully my feedback is helpful as a user though!

[jlfwong]

This is an interesting idea! As far as I can tell, if someone were to implement the selector(a) syntax here, it would still have the problems I described in #10 (comment), but since it's not an official part of the API, maybe it's okay to let people shoot themselves in the foot....? Seems not super great, but I still haven't though of any solution for descendant selection that doesn't have that problem, except for the complexity introduced in #61.

But the idea of allowing for plugins, thereby allowing Aphrodite's core to stay nice and small appeals to me. I would really love Aphrodite to be the variety of project that can be called "done" and stop development on it, leaving it in a healthy, working state.

As for the dependency issue, here's my suggestion, which also addresses my concern about the "no way to undo this".

// aphrodite-with-extensions.js
import {StyleSheet, css} from 'aphrodite';

export default StyleSheet.extend([customSelectorHandler1, customSelectorHandler2]);

Then to use the version with extension, you use aphrodite-with-extensions instead of aphrodite:

import {StyleSheet, css} from './aphrodite-with-extensions.js'

Where StyleSheet.extend does not mutate. Is that workable?

[kentcdodds]

I love the extend concept @jlfwong. Makes dependencies explicit and allows it to happen at style creation time.

[kentcdodds]

One other thing to support the idea of not changing the default handlers from aphrodite globally is if I am using aphrodite in my app and two of my dependencies are using aphrodite, they could potentially muck with each other's handlers. Or, even more likely, they could define handlers that conflict with one another. Either way, this would be highly unexpected, so doing something like @jlfwong is suggesting would be fantastic.

[xymostech]

I love the extend idea. It might make index.js a little funky but that's fine. It promotes a really clean way for people to extend stuff though. And it also adds a really obvious place for for us to add other extensions...

(maybe stringHandlers next? People also seem to want some sort of "style extends other style" syntax, which we could maybe pull off in an extension? who knows!)

[xymostech]

Okay, I finally got around to updating this pull request. Getting the StyleSheet.extend thing to work was a bit trickier than I expected it to be, but I came up with a approach with a new exports.js file that handles generating the library exports. Let me know what you think @jlfwong @kentcdodds!

Also, reminder to myself that I should write something in the readme about this before it lands.

[kentcdodds]

What would these arguments look like in this situation:

{
  foo: {
    '^bar': {
      '^baz': {
      }
    }
  }
}

Just trying to think of edge cases to make sure that the plugin system makes sense.

[xymostech]

Let's try it! I edited the test to be

const sheet = newStyleSheet.create({
    foo: {
        '^bar': {
            '^baz': {
                color: 'orange',
            },
            color: 'red',
        },
        color: 'blue',
    },
});

and it generates:

.foo_1a51bwt {
    color:blue !important;
}
.bar .foo_1a51bwt {
    color:red !important;
}
.baz .bar .foo_1a51bwt {
    color:orange !important;
}

Looks like what we'd expect?

[kentcdodds]

I'm not certain that I really understand all the code behind this (I've still yet to really take the time to learn the codebase), but as long as the API it exposes is flexible enough to support a good plugin system then I say 👍

[jlfwong]

Nice! I really like how this turned out.

This is going to need new documentation both on how to use the feature and with some warnings about how you can shoot yourself in the foot with this feature, since you'll be able to create syntax extensions with non-deterministic behavior due to injection order.

This also could be used to allow people to generate global styles, right?

function globalStyles(selector, baseSelector, callback) {
    if (selector[0] !== '!') {
        return null;
    }
    return callback(selector.slice(1));
};

function descendant(selector, baseSelector, callback) {
    if (selector[0] !== '^') {
        return null;
    }
    return callback(`.${selector.slice(1)} ${baseSelector}`);
};

Would love to get the docs knocked out as part of this diff too, since documenting it might reveal certain bits of non-obvious awkwardness.

I'm a little worried about how much firepower this gives for people to shoot themselves in the foot. People are already doing that with the StyleSheet.create({foo: {':hover .child': {...}}) hack anyway, but I'm a bit concerned that this might seem like it's providing standardized foot gunpowder.

This also opens people up to do dangerous things in their selectors like generating non-deterministic selectors by using global information (like feature detection).

I think all of these things are fine so long as we're careful to preface the "Advanced: Extensions" documentation with a scary warning about what can go wrong.

[jlfwong]

Amazing docs 😍

[jlfwong]

Is there a reasonable middle ground here using partial application to avoid needing stringHandler functions to know anything about selectorHandlers? I guess it's a little awkward because the function signature ends up being really similar to generateCSS, but not quite?

[xymostech]

I guess I mostly wrote this TODO because I think this would be another pretty easy place to hook up another extension bit, but it isn't ready for that yet because our current versions use a bunch of internal functions and state.

Is there a reasonable middle ground here using partial application to avoid needing stringHandler functions to know anything about selectorHandlers?

This is what I was trying to convey in this comment, you did it much better than I did. :)

[jlfwong]

Is this how you document this? To me, this jsdoc reads like extend takes 2 args.

[xymostech]

This is how http://usejsdoc.org/tags-param.html#parameters-with-properties says to do it!

[xymostech]

This is going to need new documentation both on how to use the feature and with some warnings about how you can shoot yourself in the foot with this feature, since you'll be able to create syntax extensions with non-deterministic behavior due to injection order.

I'm a little worried about how much firepower this gives for people to shoot themselves in the foot. People are already doing that with the StyleSheet.create({foo: {':hover .child': {...}}) hack anyway, but I'm a bit concerned that this might seem like it's providing standardized foot gunpowder.

Yes to all of that! We're definitely going to have to put some warning labels up around this, so if things don't work then they know it's because the extensions can do buggy things, and not because Aphrodite has problems.

Would love to get the docs knocked out as part of this diff too, since documenting it might reveal certain bits of non-obvious awkwardness.

The last step of this diff is to write the docs, I just didn't want to do that if we were going to change everything around again. :)

This also could be used to allow people to generate global styles, right?

I believe so! (ping #139) Your example would be a little awkward in the styles, though:

StyleSheet.create({
    '!#blah': {},
    '!body': {},
    '!.my-class': {},
});

I guess we can bikeshed what that extension would look like later. :P (other random question: if we made "official" extensions, would we want them to live in this repo or would we make separate repos for them?)

[jlfwong]

Yep, API looks good to me. Write them docs!

For the global styling, it would be:

const styles = StyleSheet.create({
    foo: {'!#blah': {}},
    bar: {'!body': {}},
    baz: {'!.my-class': {}}
});

Then injection would happen with css(styles.foo), and the returned value of that would be completely useless (so it'd be side effect only).

I guess we can bikeshed what that extension would look like later. :P (other random question: if we made "official" extensions, would we want them to live in this repo or would we make separate repos for them?)

Would prefer they live in other repositories, or for "official" extensions to just straight up not exist :)

[kentcdodds]

Would prefer they live in other repositories, or for "official" extensions to just straight up not exist :)

Agreed 👍 that would make the most sense. If a large community of extensions is developed around aphrodite, perhaps an aphrodite org with "official" extensions could reside in that org. But that's not something to think about right now I'd say.

[xymostech]

Okay, I added some docs, and changed the API a little bit to make it so people using the extensions don't need to know what kind of extension it is. Let me know how the docs look! (and how the API looks?) Maybe now that we have so many jsdocs I should figure out how to actually turn those into something people can look at...

[kentcdodds]

Maybe now that we have so many jsdocs I should figure out how to actually turn those into something people can look at...

You might try https://doclets.io/

[xymostech]

@kentcdodds Interesting! https://doclets.io/Khan/aphrodite/add-to-doclets Pretty easy to set up :) Also, looks like maybe our comments aren't formatted correctly? Some of the docs there look weird like the return value of https://doclets.io/Khan/aphrodite/add-to-doclets#dl-generateCSSRuleset which is probably our fault.

[jlfwong]

I think this could be aided a lot by an example after the section on Creating extensions so extension authors know exactly the kind of trouble they can run into.

[mattkrick]

OH BOY, i love this, it's waaay better than my quick n dirty #148.
One concern I have is that it easily allows folks to write in global styles right next to their modular ones (I believe this was eloquently described as "foot gunpowder"? 😆 ).

I do think Aphrodite greatly needs the ability for globals. And I can certainly see the benefit for nested selectors. That gives us 4 desired features:

1. @font-face, @media
2. Nested selectors
3. 3rd party css overrides
4. resets / tag defaults (eg body: {margin: 0})

Correct me if I'm wrong, but this PR would solve 1 & 2. The door would be left open for solving 3 & 4, but as mentioned, the API would require proprietary prefixes like ! and ^ to achieve that. Certainly possible, and hey, maybe we could create a standard for what those prefixes mean in JS land.

However, I'd argue that by adding an extra function to the API, say cssGlobal, that we could take away their foot gunpowder and eloquently solve 3 & 4 without the need for a prefix. This would also enable us to solve 1 the right way, instead of having the user write a modular fontFamily style that aphrodite magically turns into a global @font-face. No magic, no proprietary symbols to memorize, no lost toes.

[xymostech]

@jlfwong I added an example of generating an extension so the warnings are clearer. What do you think?

[xymostech]

@mattkrick Since adding extensions is isolated from the rest of aphrodite, you could somewhat trivially implement cssGlobal on top of this:

import StyleSheet from "aphrodite";

const myGlobalExtension = {
    selectorHandler: (selector, _, generateSubtreeStyles) => {
        if (selector[0] !== "*") {
            return null;
        }

        return generateSubtreeStyles(selector.slice(1));
    }
};

const {css, ExtendedStyleSheet} = StyleSheet.extend([myGlobalExtension]);

export default function cssGlobal(globalStyles) {
    const styles = {};    

    Object.keys(globalStyles).forEach(key => {
        styles['*' + key] = globalStyles[key];
    });

    css(StyleSheet.create({
        global: styles,
    }).global);
}

and then use it like

import cssGlobal from 'css-global.js';

cssGlobal({
    'body': {
        margin: 0,
    }

    'a': {
        textDecoration: 'none',
        ':visited': {
            color: 'blue',
        },
    },
});

[xymostech]

Random thoughts: I'm now realizing that in this implementation, the css function is the one that has to have the extension, not the StyleSheet one. This means that if you import a component that lets you pass in extra styles and it calls css(), the styles that you pass in aren't going to use whatever extensions you have added, they're going to use the ones that the component has imported.

So for instance, if I do something like

import {StyleSheet} from './my-extended-aphrodite';
import {ExternalAphroditeComponent} from 'external-library';

const styles = StyleSheet.create({
    foo: {
        color: 'red',
        '#blah': { // Something handled by extensions in my-extended-aphrodite
             color: 'blue',
        },
    },
});

const MyComponent = (props) => <ExternalAphroditeComponent styles={[styles.foo]} />;

then my extended things aren't going to be handed, right? I'm not sure what an appropriate way to reconcile these things is. 🙁 I'll think about this later...

[aaronjensen]

We added a new function locally to create a scoped css rules. The idea is you call globalCssClass('foo', { '.bar': { ... } }) and that will return a class name. You toss that class on a wrapping div (or your body, or whatever).

export const globalCssClass =
  (scope: string, sheetDefinition: StyleSheetDefinition<*>): string => {
    const hashedScopeName =
      process.env.NODE_ENV === 'test'
      ? scope
      : `${scope}_${hashObject(sheetDefinition)}`

    const styleSheets = map(sheetDefinition, (_definition, name) => ({
      _name: `${hashedScopeName} ${name}`,
      _definition,
    }))

    styleSheets.map(sheet => css(sheet))

    return hashedScopeName
  }

[jlfwong]

This line wouldn't do anything, right? It's only the return value of this that would contain the extension?

[xymostech]

Ah yes, that is correct. I'm not sure how to indicate that. module.exports =? const {StyleSheet, css} =?

[jlfwong]

Hrm, I'm not sure I'd say it's non deterministic -- I think this falls into what compilers typically refer to as "undefined behavior", right? So we can say that it's undefined behavior whether the divs will be red or blue.

Given this precise code listed here, the result will be deterministic, no?

[xymostech]

Yes that is correct. "Undefined behaviour" sounds better.

[jlfwong]

@aaronjensen as an FYI, the code you have there uses Aphrodite internals and is not guaranteed to continue working in future versions of Aphrodite (the _name and _definition properties there are prefixed as an indication that they are internal and not part of the public API).

@mattkrick It's not just about removing extensions, it's about conflicts.

Imagine that waaaay down in your stack, in a component that's used in a component that you didn't write, happens to use Aphrodite, and it happens to use a plugin that wants to use the same syntax as a different plugin that you've written. You really, really don't want those two instances of Aphrodite interacting.

@xymostech I think that case is fine. As much as possible, I'd really like things that use Aphrodite to be doing so as an implementation detail, and not expose that fact to the world, so styles being passed between two different instances of Aphrodite should be a rare occurrence. If we really wanted to enforce this, we could tag styles created with StyleSheet.create and assert that they were created by the same extended version of StyleSheet as css is using.

This diff now LGTM, but it needs conflict resolving. Excited to get this landed!

[xymostech]

I think that case is fine. As much as possible, I'd really like things that use Aphrodite to be doing so as an implementation detail, and not expose that fact to the world, so styles being passed between two different instances of Aphrodite should be a rare occurrence. If we really wanted to enforce this, we could tag styles created with StyleSheet.create and assert that they were created by the same extended version of StyleSheet as css is using.

Okay. I'm still a little hesitant, but I guess if people start complaining we can re-evaluate.

[jlfwong]

@xymostech If you can think of a reasonable way to have the transformation take place at StyleSheet.create time instead of css time, I'd also be pretty happy with that, but any path that requires us to singleton-ize Aphrodite is no good, IMO.

[xymostech]

Okay, rebased and squashed! I'll merge and release a new version.

[kentcdodds]

Is there documentation for this yet? I think I need to build a plugin to support RTL (like https://github.com/cssjanus/cssjanus does)

[jlfwong]

@kentcdodds Is https://github.com/Khan/aphrodite#advanced-extensions enough to get started?

I actually don't know if our extension API is sufficiently powerful to do automatic RTL conversion though.

[kentcdodds]

Ah, somehow missed that. Thanks @jlfwong!

I just realized... This appears to only support modifying selectors, not the styles themselves. :-( So I don't think that supporting RTL transformation is possible at the moment :-(

[jlfwong]

@kentcdodds Well, you get the CSS string back from generateSubtreeStyles, which you could do arbitrary operations on. That's not exactly a convenient API, but it would allow you to do things like this by reparsing the CSS and changing it. I haven't thought this all the way through, but you might run into troubles since generateSubtreeStyles will also use your extension, I think?

[kentcdodds]

Yeah, also it probably wouldn't perform well. I'd rather just manipulate the objects themselves before I pass them to aphrodite. I think that I'm going to do that.

[jlfwong]

@kentcdodds Ah, yeah! That's probably a good approach.

Something like this?

const styles = StyleSheet.create(rtlConvert({
    something: {
        textAlign: 'left'
    },
}));

[kentcdodds]

Yep, I wish that something like that existed already, but I don't think it does :-(

[mattkrick]

@kentcdodds i'm guessing your usecase is when someone switches a language? In that case, you can just treat it like a theme. You have your theme/language passed in via context, you memoize on the context in a withStyles HOC, and when the context changes it invalidates all styles for every component & you're now rockin rtl.

[xymostech]

@kentcdodds Really looking forward to seeing that! Maybe something like that "traverse-and-modify" will become a more common tool. Maybe the transform could even be done using a babel plugin, so it's not done on the fly?

@mattkrick You could treat it like a theme, but the point of doing an rtl conversion is that you can automatically figure out what the new styles will be based on the old styles. I think what you're suggesting would require re-writing all of the styles for both ltr and rtl instead of automatically converting.

[kentcdodds]

I started it here: https://github.com/kentcdodds/rtl-css-js

Everything's ready to start adding tests and making them work. Check the issues :)

[natew]

Are there any extensions out there already? Links to examples in the docs would be reassuring to people who are looking. And maybe suggest a standard naming convention to make it easier to find aphrodite- or aphrodite-extension-

[merlinstardust]

What's the recommended way to do descendant selectors?

Is it just the already existing (but unintentional) :hover .child or is it better to use the suggested descendant handler above?

[Baggz]

@jlfwong @kentcdodds @xymostech

Hi, What's the recommended way to do descendant selectors? E.g. .parent:hover .child? Shall I create a custom extension?

[jlfwong]

@Baggz I think https://github.com/Khan/aphrodite#advanced-extensions contains example code for one way to tackle that. I'm not aware of a canonical solution, however.