Aphrodite generates invalid CSS selectors

[wereHamster]

http://stackoverflow.com/questions/448981/which-characters-are-valid-in-css-class-names-selectors

Aphrodite happily generates invalid selectors. Here's an example where I wanted to generate a map from DimensionId (a two-digit number) to a style object which sets the color:

const foregroundColor = StyleSheet.create({
    ['01']: { color: dimensionColors['01'] },
    ['02']: { color: dimensionColors['02'] },
    // ... up to '12'
});

const dimensionId = '01'; // Loaded from a database.
const className = css(foregroundColor[dimensionId]);

// -> className === ".01-???", starts with a number.

Either warn the user that the style is not going to work, or better, prefix selectors with a letter to make them always valid.

[kentcdodds]

I'm not convinced that aphrodite should have to handle this case. But this isn't my project. If the maintainers want to accept a PR to warn against this that's fine with me. It'd be handy for sure.

[jlfwong]

@wereHamster Thanks for the report!

Sounds like you ran into some frustrating debugging experiences here.

I can see how that's annoying, but I agree with @kentcdodds here. In the interest of keeping the project small (and the code size small), I'd rather not do extensive validation. We similarly don't warn if you try to use CSS properties that don't exist. Aphrodite will also quite happily generate garbage CSS if you do this:

StyleSheet.create({a: { zubat: '1px' }});

And I don't want to go down the rabit hole of extensive validation.

If we did want to go down that rabbit hole, I would certainly want to set up separate development and production builds first, ala React, as described in #88.

[sophiebits]

Really? Why would you expect that a stylesheet key needs to be a valid classname? Requiring people to understand that requires them to understand implementation details of Aphrodite. zubat: 1px; wouldn't be garbage; it'd be exactly what the user asked for.

[jlfwong]

Perhaps zubat was a bad example. A better example would be bordreRadius: 1. It's almost definitely not what the user wanted, and will fail silently. But I see your point.

I'm okay with the property key and the generated classname not exactly matching up, I suppose. Prefixing with a- if the name doesn't start with a letter, and stripping out characters that aren't valid in a classname would be fine with me.

Instead of prefixing, we could also just flip the order of the human readable name and the hash. While the generated class names is not strictly part of the API, I can imagine someone having tooling that relies upon that fact, so I'd rather not change that.

Assuming the change can be kept small, I'm cool with accepting a PR implementing that w/ tests for the functionality.

[kentcdodds]

While the generated class names is not strictly part of the API, I can imagine someone having tooling that relies upon that fact, so I'd rather not change that.

👋 yeah, I've got E2E tests that use that. It'd be easy for me to fix that. Generally I think it's better to add a _test-foo class to elements needing that kind of functionality anyway, so I'd be fine with that kind of a change.

[xymostech]

I'm okay with the property key and the generated classname not exactly matching up, I suppose. Prefixing with a- if the name doesn't start with a letter, and stripping out characters that aren't valid in a classname would be fine with me.

This sounds good to me. It doesn't change existing class names and fixes the problem. We could prefix with something fun like a unicode character or maybe just _?

:wave: yeah, I've got E2E tests that use that. It'd be easy for me to fix that. Generally I think it's better to add a _test-foo class to elements needing that kind of functionality anyway, so I'd be fine with that kind of a change.

@kentcdodds we've been making do in our e2e tests by adding data-test-id="blah" attributes to our elements. That way, we don't have to combine class names.

[kentcdodds]

we've been making do in our e2e tests by adding data-test-id="blah" attributes to our elements. That way, we don't have to combine class names.

Good call 👍

What if we prefix all class names with _? It'd be nice to not have to add logic and still solve the problem...