Add experimental 'substitute' option, that will subtitute all template literals

[bartlangelaan]

This (experimentally) solves #16.

When this package is used, now an option can be specified:

// Before
const syntax = require('@stylelint/postcss-css-in-js')();

// After
const syntax = require('@stylelint/postcss-css-in-js')({ unstable_substitute: true });

This is marked explicitly as an experimental option, as the way this feature is implemented is probabily not the best way to do it, and existing implementations can fail on this option being set to true. This also makes it possible to change the behaviour of this option between semver-compatible releases.

The naming of unstable_ is how Next.js also implements experimental features, which is why I named it like this (there are no existing options for this processor currently).

Even though this is probabily not the best way to build something like this, I would still propose to merge and release this. This makes it possible to run a lot of stylelint rules reliably - it only fails on generating good-looking error messages. For stylelint it will look like the template literals never existed.

---

Merge request stylelint/stylelint#4944 enables this as an unstable option in stylelint - its last commit can be used to easily test this out in your projects.

[hudochenkov]

That is some great work you did!

If I understood correctly it replaces all template literals with $dummyVariable. Similar how stylelint-processor-styled-components does. Is it correct?

What are regressions we could expect if we enable new behavior by default?

However, the error messages can be very unexpected - it will for example show "$dummyValue4" in the error messages, instead of the code of the user.

This messages would be false positives, if I understand correctly. Stylelint rule will read AST and see something like animation-delay: $dummyVariable1s and report that. Which is not correct. We might adjust rules to ignore $dummyVariable, but we might need to do this to every rule, which is not realistic. We might also filter our violations if we find $dummyVariable in the message. This sound more realistic.

[bartlangelaan]

That is some great work you did!

Thanks! 🙏

If I understood correctly it replaces all template literals with $dummyVariable. Similar how stylelint-processor-styled-components does. Is it correct?

Yes!

This messages would be false positives, if I understand correctly.

No, actually not. I have tested this on some large styled-components projects, and so far most messages are legit.

For example, if you have this file:

import styled from 'styled-components';

const A = styled.div``;

const B = styled.div`
  ${A} & {
    color: red;
  }
  ${A} & {
    color: green;
  }
`;

This will produce the following warning:

Unexpected duplicate selector "$dummyValue0 &", first used at line 6 (no-duplicate-selectors)

The error is 100% legit - but the error message is very unexpected. That will need some work - but by already offering it as an unstable option, it can be used by users that know how to handle these type of messages.

We might also filter our violations if we find $dummyVariable in the message. This sound more realistic.

There are already some helper functions that check for SASS-like variables. For example, the isStandardSyntaxMediaFeatureName will already filter out media-queries that contain the $ character. I don't know if more exceptions are needed - so far it's looking okay.

What are regressions we could expect if we enable new behavior by default?

Most of all, it's possible that rules that didn't work correctly before now work, which can generate new errors.

Also, probably not all features and edge cases will work before extensive testing in the wild. It's easier to make breaking changes while this is released under an unstable tag, and by releasing it to NPM it makes it very accessible for project owners to test this feature in their project.

[hudochenkov]

There are already some helper functions that check for SASS-like variables. For example, the isStandardSyntaxMediaFeatureName will already filter out media-queries that contain the $ character. I don't know if more exceptions are needed - so far it's looking okay.

Right! Forgot about these helpers. In this case example that you showed with duplicated selectors won't be reported, though. On the other side we do use use these helpers extensively, if non-standard syntaxes causing problems.

---

I have an another idea. When template literal replaced with a substitution in PostCSS AST, we could add original content to the node somewhere. When rule report it passes message and the node. So in report we could check if message has $dummyValue, and it it has, then replace it with original value, which we take from the node.

[bartlangelaan]

In this case example that you showed with duplicated selectors won't be reported, though.

Currently, it does! And now that I think of it... that's both a feature and a bug. Right now, it uses the index of the property as $dummyValue id. And it also de-duplicates them, so if you use the same template literal in the same property, then they will have the same id.

This is great if you want to report stuff like #test ${A} ${A} - then the rules know that both interpolations are the same.

That however also generates the case where this code:

export const a = styled.div`
    ${ButtonStyled} {}
    ${ButtonStyled2} {}
`;

Will have the same error:

Unexpected duplicate selector \"$dummyValue0\", first used at line 3 (no-duplicate-selectors)

So this ID needs to be more unique - enumeration based on occurrence in the same root should be enough.

So in report we could check if message has $dummyValue, and it it has, then replace it with original value, which we take from the node.

That's actually a great idea! I don't know if that will always work - if a rule uses information from another node to produce the message, that exact node may not contain the interpolation data. But it will for sure work for a lot of messages.

---

I feel like the real, best solution would be to fix this inside the tokenizer and work with 'raw' values. But I'm not that into postcss, and I just don't understand the code well enough to adjust how the tokenizer works. The concept of 'raw' values is still very vague to me. So if somebody else passes along and does know how that works, the code can always be rewritten so 'hacks' like parsing report messages won't be necessary

[hudochenkov]

So if somebody else passes along and does know how that works, the code can always be rewritten so 'hacks' like parsing report messages won't be necessary

No one know how does it work :) Person, who created it is not active on Github anymore.

Any solution is a good solution, if it solves problems.

[ota-meshi]

Does this PR change break the auto-fix?

[bartlangelaan]

Does this PR change break the auto-fix?

No, it should not.