Decide if replacing expressions with a CSS comment is the right decision

[43081j]

If we get some really funky looking interpolation, we may end up producing invalid syntax somehow:

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

in this case, it is likely the author's code always sets a color. But at compilation time, we have no idea if this is the case or not.

We will produce:

.foo {
  color: /*POSTCSS_LIT:0*/;
}

which means foo no longer has a colour. i wonder if this could inadvertently trigger other plugins (e.g. a plugin to remove dead code, empty declarations).

without having a special case for every possible expression position, im not sure if there is a better way to what we do now, though.

[Garbee]

This is a pretty huge deal for lit projects that import design tokens from an external object to use. For example:

import {tokens} from 'internal-design-system';
import {unsafeCSS,css} from 'lit';

// Tokens being an object of key/value pairs where values are CSS property values.

export const baseStyles = css`
button {
  /* before */
  background-color: var(--app-button-color, ${unsafeCSS(tokens.brandColor)});
  /* after */
  background-color: var(--app-button-color, /*POSTCSS_LIT:0*/);

  /* before */
  color: var(--app-button-text-color, ${unsafeCSS(tokens.primaryWhite)});
  /* after */
  color: var(--app-button-text-color, /*POSTCSS_LIT:1*/);
}
`;

As of right now, anytime someone tries to auto-fix files in the app, these will all break. The values even persist into things like opacity values, box shadows, border styles, etc. So the effects are quite destructive meaning the auto-fixer can never be used reliably.

This is something that IMO should absolutely be fixed so any interpolation items end up in place after formatting as they were before the fix.

[43081j]

yep, it needs a smarter placeholder based on the token type.

it isn't a trivial problem to fix so will take some time unfortunately, but i'll try have a look at it soon

[Garbee]

I'm going to try and spend some time this weekend looking into this. Since the numbers are incrementing, I'm hoping an array is saved all the strings. If that's true, then if we can get to it when saving we'd need to inject that back into the source. Not perfect, but at least non-destructive if it can be pulled off.

[43081j]

a postcss syntax defines a parser and a stringifier such that auto-fixing results in this flow:

source -> parser -> stringifier -> fixed source

during parsing, we extract template strings, replace the expressions with placeholders, then parse the result through postcss' own parser.

css`
  .foo {
    ${v}
  }
`;

becomes

css`
  .foo {
    /*POSTCSS_LIT:0*/
  }
`;

a solution is to look behind when choosing which placeholder type to use (e.g. if the previous non-whitespace string was a :, we're probably in a "value").

the old postcss plugins (before syntaxes existed) did this.

otherwise, we'd need to write our own "css/js" tokenizer to figure out what the previous token is rather than doing guesswork with strings. but that seems like serious overkill.

having varying placeholders also means we'd have to update the location correction code (computes the equivalent source location of a CSS location and vice versa). that code is particularly fiddly.

[43081j]

spent a little time looking into this, it might be easier than expected in fact

currently we replace the expression with a comment:

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

  .bar {
    ${prop}: hotpink;
  }

  ${selector} {
    color: hotpink;
  }
`;

this results in:

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

  .bar {
    /*POSTCSS_LIT*/: hotpink;
  }

  /*POSTCSS_LIT*/ {
    color: hotpink;
  }

which of course causes all sorts of syntax errors (e.g. the empty declaration).

however, what if we just used a word?

  .foo {
   color: PLACEHOLDER;
  }

  .bar {
    PLACEHOLDER: hotpink;
  }

  PLACEHOLDER {
    color: hotpink;
  }

this is valid syntax technically and parses fine.

ironically used comments originally figuring they'd be less likely to cause syntax problems, but seems the opposite.

im sure there's still many crazy binding positions you could do to break it, but it seems better than what we have.

example of a funky one:

  PLACEHOLDER

  PLACEHOLDER {
    color: hotpink;
  }

this would probably end up treating PLACEHOLDER\n\nPLACEHOLDER as a selector. ofc you could do all sorts of weird stuff like template in half a selector...

cc @kutnickclose

[kutnickclose]

I started working on a custom syntax for linaria. I've found so far that this placeholder strategy below minimizes the number of additional custom linting rules needed.

ruleset placeholder =  /* pcss-lin:# */
atRule/Selector placeholder = .pcss-lin#
property placeholder = --pcss-lin#
value placeholder = pcss-lin#

Ruleset: comments work pretty well when the whole expression is one line AtRule/Selector: I had separate at-rule and selector placeholders initially but just combining both into a selector looking placeholder was simpler and covered more use cases (i.e. ${expr} { .. }, ${expr} .example { ... }, .example ${expr} { ...}) Property: the custom property won't throw that it's not a valid property Value: just tried to keep it relatively short so it won't throw max-char errors All: lowercase to avoid those lint rules that say you shouldn't have capitalized letters

I may also want to remove the hyphen but testing has been fine so far.

[43081j]

to be honest, the choice of placeholder isn't the difficulty here (e.g. PLACEHOLDER is as valid as all the ones you wrote, in the same positions).

detecting the binding position is the problem. knowing that this:

.foo {
  ${prop}: hotpink;
}

is in a property position.

we could lazily look behind and guess, e.g. based on if there's a semi or a bracket somewhere previously. but i feel like there will be numerous edge cases as with any guess-work solution.

[timbomckay]

I ended up copying this plugin locally while waiting for some of these issues to be resolved. Updated parse.js to run on postcss and added it to stripStyles.js so Tailwind would ignore the new tagged template. This way we can use the css tagged template as expected from Lit and only process styles through postcss that we declare necessary.

So a styles file might look something like this:

import { css } from 'lit';

/** Used to process through the PostCSS config via postcss-lit */
export const postcss = css;

/** Cycles through the heading levels to provide custom property variables for each level */
const headings = [...Array(6).keys()].map((i) => css`h${i + 1}, .nb-h${i + 1} {
  color: var(--nb-h${i + 1}-color);
  font-size: var(--nb-h${i + 1}-font-size);
  font-weight: var(--nb-h${i + 1}-font-weight);
  line-height: var(--nb-h${i + 1}-line-height);
  margin-bottom: 0.5em;
  margin-top: 0;
}`);

export const base = [
  css`:host { box-sizing: inherit; }`,
  ...headings,
  postcss `@tailwind base; @tailwind utilities;`,
];

[43081j]

Yeah which I'm sure you see doesn't solve the overall issue here, rather works around it.

So I think I'm still gonna have to figure this out at some point. I'm low on time recently but I'll try have a look this week.

There will be a simple solution I'm missing, I'm sure.

[timbomckay]

Right, yeah definitely a work around for the time being. Just didn't see a way to move forward without it.

One thought is to add the syntax option feature so users can add/replace the targeted syntax. This would provide the workaround in a new feature while providing time to figure out the solution to this issue.

[43081j]

FYI im still trying to figure this out...

even writing a small state machine to know the rough position has exploded into not-so-small anymore.

i think i need a dumber solution, much simpler. like just guessing if we're in a property or not maybe...

there are a few of you waiting on this now im aware too.

fwiw both the eslint lit plugin and the template literal minifier (from the lit team) have the exact same problem to solve. so seems its just not a simple one in general