feat: add stricter expression placeholders

[43081j]

Fixes #6.

Briefly, the problem we've had all along is that there's no one placeholder which will produce valid syntax for all positions an expression may be interpolated into.

This can't ever really be solved perfectly as we have no idea until run-time what the actual interpolated value will be, and what position it is in.

Until now, we use a CSS comment as a placeholder, but this means something like the following will not work:

css`
  .foo {
    color: ${expr};
  }
`;

The reason is, this will become:

.foo {
  color: /*PLACEHOLDER*/;
}

Which, from an AST point of view, is the same as having a value-less declaration. This can cause all sorts of problems.

In this rework, we instead try to guess what kind of position we're in based on looking ahead/behind then choosing the right placeholder (syntax) for that position.

This means we no longer support all binding positions. We didn't anyway, since errors were thrown in many cases, but now we actually have a known constraint around this.

As long as you're interpolating complete pieces of syntax in, everything should be fine. For example, these should all be fine:

css`
  ${expr} {
    color: hotpink;
  }

  .foo {
    color: ${expr};
  }

  .foo {
    ${expr}: hotpink;
  }

  .foo {
    ${expr}
  }
`;

While these are all near impossible for us to understand within the linter so are no longer allowed:

css`
  .foo, ${expr} {
    color: hotpink;
  }

  .foo {
    prop: ${expr}-abc;
  }
`;

These are all partially interpolated pieces of syntax: part of a selector, part of a value, etc. Which will no longer work (if it ever did).

cc @stramel @timbomckay @Garbee @kutnickclose in case any of you are still interested/curious about this, have any feedback, etc.

ill publish a prerelease so people can try it out soon

[coveralls]

Pull Request Test Coverage Report for Build 3554140206

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 3 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+0.8%) to 96.828%

---

Files with Coverage Reduction	New Missed Lines	%
lib/parse.js	1	93.69%
lib/util.js	2	95.83%
<!--	Total:	3		-->

Totals
Change from base Build 2974909770:	0.8%
Covered Lines:	836
Relevant Lines:	843

---

💛 - Coveralls

[timbomckay]

That's progress I suppose but don't think it'll work with our given scenario. We have loop for heading styles to insert custom properties. Instead of specifying each level we use an array that looks something like this:

const heading = () => [...Array(6).keys()]
    .map((i) => css`h${i} {
        color: var(--h${I}-color);
        ...
    }`);

[43081j]

as long as you interpolate the complete syntax, it should work:

const heading = () => [...Array(6).keys()]
    .map((i) => css`${`h${i}`} {
        color: ${`var(--h${I}-color)`};
        ...
    }`);

its not ideal in that case because you're doing the one thing no approach can ever really cover: partial interpolation of syntax.

by requiring complete syntax nodes, we can at least more safely guess what kind of position we're in based on our surroundings.

i've tried so many different approaches over the last month or so, and had various catch ups with people (e.g. some of the lit team as they have the same problem elsewhere). this is the cleanest i could come up with. it is an incredibly difficult problem a few packages have right now and nobody has solved so far, this is a lot of progress.

we can never know as much as you (the user) knows, or the run-time environment knows.

[timbomckay]

Yeah I might need to rename the literal function for this particular function so that postcss-lit doesn't try to parse it.

const skippostcss = css;

const heading = () => [...Array(6).keys()]
    .map((i) => skippostcss`h${i} {
        color: var(--h${I}-color);
        ...
    }`);

Sorry, I realize I'm providing a lot of noise at this point on the PR. I think this renaming should work though to alleviate the issue for me, essentially telling postcss-lit not to parse this section.

[kutnickclose]

(context: Linaria now has a custom syntax based on this one)

I was able to account for the partial-interpolation I believe with the strategy of "if it's not a selector, property, or ruleset then it's either a value or partial-interpolation (which can use the same placeholder)". One of the changes I had to make was making the placeholder aware of the whole line rather than just what's immediately before it or after.

code here: https://github.com/callstack/linaria/blob/master/packages/postcss-linaria/src/util.ts#L59

[43081j]

Yeah I might need to rename the literal function for this particular function so that postcss-lit doesn't try to parse it.

you can do that or you can just interpolate complete syntax like i showed you (put it in a variable if you want it to look nicer).

I was able to account for the partial-interpolation I believe with the strategy of "if it's not a selector, property, or ruleset then it's either a value or partial-interpolation (which can use the same placeholder)"

its one of the many solutions i considered (and tried) over the last month but wasn't quite enough.

the amount of edge cases grows quickly. for example, one from a glance:

css`
  .foo::${expr} {
  }

  ${expr}:hover { /* foo */
  }
`;

there's so many more.

the solution i have here would support far more as long as people know to only ever interpolate complete pieces of syntax.

on a side note, im surprised any of what you have worked because the location correction code had a good, non-obvious bug in it which you should've run into when you changed the placeholders. well done dodging that :D

[kutnickclose]

Ya makes sense. Seems like the options are to 1) restrict some ways of development (i.e. disallow partial interpolation) with more accurate reporting and less bugs or 2) live with a long tail of edge cases. Allowing partial interpolation was a requirement for me but perhaps an eslint/stylelint rule could warn users using postcss-lit like "hey, don't partially interpolate this."

[43081j]

Allowing partial interpolation was a requirement for me

curious, what did that requirement come from?

would be good to see some examples where you can't possibly do it any other way. like in the example further up, you can avoid partial syntax just by being more careful/specific with the expressions in most cases.

[kutnickclose]

That requirement came from working on a large code base where the partial interpolation is/was a common occurrence. And I think it's totally doable to avoid partial interpolation, just getting many engineers to remember/align on that is the difficult part.

[43081j]

ok cool that's actually good to hear, since it means its for consistency rather than a technical limitation.

it does seem a shame that in a codebase like yours, this would just be disabled if i ship the change. but on the other hand, if i don't ship the change or i open the flood gates to edge cases, we're in an equally (if not worse) situation with potential bugs.

there's no ideal solution to this.

from a clean repo, this solution is great. but from an existing, large repo, not so much... although this plugin in particular shouldn't have worked very well beforehand in such a repo anyway.

[kutnickclose]

Ya, for clarification, I use @linaria/postcss-linaria as postcss-lit didn't quite meet my requirements. I'm sure this PR would be welcomed by those using postcss-lit currently since it adds additional functionality.

[43081j]

it does raise the point still though that others out there will have stylesheets just like yours, with partial syntax in them.

the current behaviour supports some of it (not by design) so we would be regressing slightly in order to move forward in all other cases. tough one.

on a side note @kutnickclose it does seem far from ideal that you're now going to be maintaining the same core as what I provide here, missing out on any bug fixes, updates, reworks, etc. is the only difference your placeholder implementation?

some time ago I did start extracting the core of this into a shared library, maybe that's what you should be using rather than forking the repo. then you could maintain your code and still depend on the same core as this package.

if that sounds like the right way to go, im happy to jump back on that task and get it published

[kutnickclose]

Ya, I wasn't sure how quickly you might update postcss-lit and wasn't sure if any updates would fit my requirements so forking was the best option at the time. I think having a "core" library could definitely help support other css-in-js libraries (including Linaria)! It would be great if lit/emotion/styled-components/linaria/etc could share code.

I think the main other updates I made were to the stringifier to support the different types of placeholders.

[43081j]

I think the main other updates I made were to the stringifier to support the different types of placeholders

this will be solved generically in this update too FYI. so you're left with only the placeholder changes and the rest is identical.

i'll have another stab and extracting the core again this week before this can land.

i already started on a styled-components plugin at some point but don't use styled-components myself so abandoned it in the end. it has a lot more binding positions and syntax possibilities than regular CSS unfortunately, but could still use the same core if someone else picks it up some day. probably the same for the rest.

[43081j]

in #36 i introduced a way to disable lines from being parsed by this plugin, so that means those of you who have particularly complex strings can at least disable those individual cases.

im guessing/hoping even the common cases for you are the ones without partial syntax. so the edge cases can just be disabled.

i don't think there's any solution to large existing codebases which partially interpolate syntax. one way or another they'd need updating, whatever solution we come up with. at least this way there's a choice between being smarter about expressions, or just adding a comment.

edit:

have also now made the plugin skip unparseable templates at least. which means these edge cases won't halt it anymore.

comically it turns out this stricter logic actually does unintentionally support many of the existing partial binding positions, too. who knew. so in fact only far edge cases seem to trip it up