Base indentation detection #16

43081j · 2021-12-05T23:03:24Z

This detects the base indentation (that of the last backtick of the template) and removes it from the CSS source before parsing it.

We need this so we don't trigger stylelint's indentation rules incorrectly.

Fixes #14

coveralls · 2021-12-05T23:06:23Z

Pull Request Test Coverage Report for Build 1542386357

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.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-1.3%) to 95.9%

Files with Coverage Reduction	New Missed Lines	%
lib/locationCorrection.js	1	91.06%

Totals
Change from base Build 1508993096:	-1.3%
Covered Lines:	444
Relevant Lines:	449

💛 - Coveralls

We now store the before/after strings from both the JS and the CSS in the raws of each node. This means we can simply output the JS raws when stringifying the node.

43081j · 2021-12-06T22:47:34Z

turns out that implementation was nonsense. have updated to sort it out, should be fine now.

also added a test which simply enables stylelint's indentation rule in the hope that it'd fail if we did something wrong here

43081j · 2021-12-06T22:57:10Z

@abdonrd @web-padawan could you possibly give postcss-lit@next a try? and let me know if you run into any problems

web-padawan · 2021-12-06T23:14:07Z

Thanks, I will try it tomorrow 👍

43081j · 2021-12-07T10:00:49Z

i gave it a go, i think we still have one last problem: accounting for the first newline.

css`
  .foo {}
`;

The first new line after css will cause stylelint to complain about no empty lines being allowed at the start. so we should just ignore/strip it i think

web-padawan · 2021-12-07T13:49:27Z

@43081j Tried to use it with our web components monorepo and faced an error:

$ stylelint packages/**/theme/**/*-styles.js
SyntaxError: Unexpected token (1:0)
    at Object._raise (/Users/serhii/vaadin/web-components/node_modules/postcss-lit/node_modules/@babel/parser/lib/index.js:541:17)
    at Object.raiseWithData (/Users/serhii/vaadin/web-components/node_modules/postcss-lit/node_modules/@babel/parser/lib/index.js:534:17)
    at Object.raise (/Users/serhii/vaadin/web-components/node_modules/postcss-lit/node_modules/@babel/parser/lib/index.js:495:17)
    at Object.unexpected (/Users/serhii/vaadin/web-components/node_modules/postcss-lit/node_modules/@babel/parser/lib/index.js:3580:16)
    at Object.parseExprAtom (/Users/serhii/vaadin/web-components/node_modules/postcss-lit/node_modules/@babel/parser/lib/index.js:12026:22)
    at Object.parseExprSubscripts (/Users/serhii/vaadin/web-components/node_modules/postcss-lit/node_modules/@babel/parser/lib/index.js:11584:23)
    at Object.parseUpdate (/Users/serhii/vaadin/web-components/node_modules/postcss-lit/node_modules/@babel/parser/lib/index.js:11564:21)
    at Object.parseMaybeUnary (/Users/serhii/vaadin/web-components/node_modules/postcss-lit/node_modules/@babel/parser/lib/index.js:11539:23)
    at Object.parseMaybeUnary (/Users/serhii/vaadin/web-components/node_modules/postcss-lit/node_modules/@babel/parser/lib/index.js:9918:20)
    at Object.parseMaybeUnaryOrPrivate (/Users/serhii/vaadin/web-components/node_modules/postcss-lit/node_modules/@babel/parser/lib/index.js:11353:61)

Here is the branch: https://github.com/vaadin/web-components/tree/stylelint-14
In order to try it out, run yarn and then yarn lint:css.

43081j · 2021-12-07T16:29:17Z

strange, that sounds like there's syntax in your code that the babel parser didn't understand. ill see if i can reproduce it locally tonight

43081j · 2021-12-07T18:13:58Z

@web-padawan this is because stylelint-config-vaadin is still loading the styled-components preprocessor. that turns your stylesheet into some strange nested stylesheet, which the stylelint parser doesn't understand (since nesting isn't a thing yet).

i removed it locally and it then lints correctly, but has the same "rule-empty-line-before" problem i mentioned above, ill fix that tonight

web-padawan · 2021-12-07T19:00:39Z

Thanks for looking into it. I will put together a new version of the Stylelint config tomorrow.

We trim this so we don't trigger stylelint's rules which disallow empty first lines. In JS sources, an empty first line makes sense. We also have to set `beforeStart` explicitly on the root's raws until stylelint fixes stylelint/stylelint#5767.

43081j · 2021-12-07T20:08:15Z

@web-padawan i published a new @next build and ran it against your repo with styled-components disabled, it all passes! it finds some legitimate warnings, some spacing between keyframes which i guess is an improvement or change in stylelint's rules since 14.x.

I had to put a bit of a hack in my PR which has been written up here: stylelint/stylelint#5767 for anyone curious.

43081j · 2021-12-08T09:04:40Z

i've tested this against our repos too and it seems to work fine, so im gonna go ahead and merge.

if you run into any problems, can you please open another issue?

web-padawan · 2021-12-08T09:38:25Z

@43081j Thanks! Could you please also publish 0.1.3 so we can use it in the project?

43081j · 2021-12-08T10:36:24Z

i've just published 0.2.0 @web-padawan

abdonrd · 2021-12-08T23:34:37Z

@43081j it doesn't work well for me 😕

With the same branch: https://github.com/IBM/pwa-lit-template/compare/stylelint-14
If I run npm run lint:stylelint -- --fix, it moves the indentation:

43081j · 2021-12-09T08:39:30Z

@abdonrd can you possibly raise a new issue and ill try look into it tomorrow

will run it against your branch and see where i get to

abdonrd · 2021-12-09T09:03:39Z

Done! #18

web-padawan · 2021-12-09T09:04:41Z

@43081j I got it working in our web components monorepo, many thanks for putting this together! ❤️
Here is a PR in case you want to take a look: vaadin/web-components#3166.

Some interpolations were causing "comment-whitespace-inside" errors so I removed them, for example:

const richTextEditorStyles = css`
  ${contentStyles}
  ${toolbarStyles}

:host([readonly]) [part='toolbar'] {
    display: none;
  }
`;

The errors are caused by ${contentStyles} and ${toolbarStyles} lines apparently treated as comments 🤷‍♂️

43081j · 2021-12-10T14:41:52Z

ah yes, this is because we replace expressions with comments. but i opened #6 for this, as we may need to be smarter about what we use as a placeholder (e.g. values in value positions, comments elsewhere).

i'll add a test like that at some point and see what i can do to make it better

43081j added 4 commits December 5, 2021 20:13

add base indentation detection

10e6af3

account for mixed indentation

0ea8dc0

add correction for before/after strings

4eabe5e

add jsdoc to before/after correction

cc2bdda

43081j added 2 commits December 5, 2021 23:14

add test for single line css

bdc409e

fix: rework indentation state to be computed early

bfd5b90

We now store the before/after strings from both the JS and the CSS in the raws of each node. This means we can simply output the JS raws when stringifying the node.

0.1.3-indent.0

9042542

43081j added 2 commits December 7, 2021 19:55

fix: trim leading newline

47dc4d2

We trim this so we don't trigger stylelint's rules which disallow empty first lines. In JS sources, an empty first line makes sense. We also have to set `beforeStart` explicitly on the root's raws until stylelint fixes stylelint/stylelint#5767.

0.1.3-indent.1

e8b5525

43081j added 2 commits December 7, 2021 20:11

test: add a test for multi-line css

c1f2490

test: add tests for multi-line css

a3aff58

43081j merged commit 7597da1 into master Dec 8, 2021

43081j deleted the base-indent branch December 8, 2021 09:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Base indentation detection #16

Base indentation detection #16

43081j commented Dec 5, 2021 •

edited

Loading

coveralls commented Dec 5, 2021 •

edited

Loading

43081j commented Dec 6, 2021

43081j commented Dec 6, 2021

web-padawan commented Dec 6, 2021

43081j commented Dec 7, 2021

web-padawan commented Dec 7, 2021

43081j commented Dec 7, 2021

43081j commented Dec 7, 2021 •

edited

Loading

web-padawan commented Dec 7, 2021

43081j commented Dec 7, 2021 •

edited

Loading

43081j commented Dec 8, 2021

web-padawan commented Dec 8, 2021

43081j commented Dec 8, 2021

abdonrd commented Dec 8, 2021

43081j commented Dec 9, 2021

abdonrd commented Dec 9, 2021

web-padawan commented Dec 9, 2021

43081j commented Dec 10, 2021

Base indentation detection #16

Base indentation detection #16

Conversation

43081j commented Dec 5, 2021 • edited Loading

coveralls commented Dec 5, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1542386357

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

43081j commented Dec 6, 2021

43081j commented Dec 6, 2021

web-padawan commented Dec 6, 2021

43081j commented Dec 7, 2021

web-padawan commented Dec 7, 2021

43081j commented Dec 7, 2021

43081j commented Dec 7, 2021 • edited Loading

web-padawan commented Dec 7, 2021

43081j commented Dec 7, 2021 • edited Loading

43081j commented Dec 8, 2021

web-padawan commented Dec 8, 2021

43081j commented Dec 8, 2021

abdonrd commented Dec 8, 2021

43081j commented Dec 9, 2021

abdonrd commented Dec 9, 2021

web-padawan commented Dec 9, 2021

43081j commented Dec 10, 2021

43081j commented Dec 5, 2021 •

edited

Loading

coveralls commented Dec 5, 2021 •

edited

Loading

43081j commented Dec 7, 2021 •

edited

Loading

43081j commented Dec 7, 2021 •

edited

Loading