Allow disable comment to be the directly above the statement

[pndewit]

The comment line to disable the postcss-lit plugin (postcss-lit-disable-next-line) doesn't necessarily need to be on the parent statement, especially with static class fields being more and more mainstream: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/static

The following snippet breaks stylelint with postcss-lit:

const MY_CONST = 600;

export class MyClass extends LitElement {
  static styles = css`
    @media (width >= ${MY_CONST}px) {
      ...
    }
  `;
}

Errors:

• Unexpected invalid media query media-query-no-invalid
  "(width >= POSTCSS_LIT_0px)"
• Unexpected unknown media feature media-feature-name-no-unknown
  name "POSTCSS_LIT_0px"

To disable the plugin, I'd now have to disable it for the entire class because styles is a static class field and therefore the parent statement is the MyClass class:

const MY_CONST = 600;

// postcss-lit-disable-next-line
export class MyClass extends LitElement {
  static styles = css`
    @media (width >= ${MY_CONST}px) {
      ...
    }
  `;
}

Because my classes contain more CSS than just this media statement, I'd like to be able to not disable this plugin for the entire class nor do I want to replace my static class field back to a getter. This PR allows disable comments to be placed directly above the statement, but the previous behaviour of looking at the parent comments is taking precedence so that it should not be a breaking change. This now works:

const MY_CONST = 600;

export class MyClass extends LitElement {
  static styles = [
    // postcss-lit-disable-next-line
    css`
      @media (width >= ${MY_CONST}px) {
        ...
      }
    `,
    css`
      .my-class-i-want-to-stylelint {
        ...
      }
    `,
  ];
}

I also made it so the disable comment doesn't necessarily need to be the last comment before the statement. It can be one of the comments which is required if I also want to disable for example eslint for the same line, e.g.:

// postcss-lit-disable-next-line
// eslint-disable-next-line
css`
  @media (width >= ${MY_CONST}px) {
    ...
  }
`

Let me know what you think, I'm open for suggestions!

---

EDIT:
Future work: maybe think about also supporting the following syntax:

css`
  /* postcss-lit-disable-next-line */
  .foo { color: hotpink; }
`;

As far as I can see the CSS tagged template expression is not parsed by Babel though, so it might require using another library for that.

[43081j]

seems to make sense to me

I think it was done originally because we may be looking at a deeper node and wanted to look for comments on the owning statement instead

looks like we're still doing that with this change so should be fine

[pndewit]

Yep, tried to keep the current logic as the first check so it wouldn't introduce breaking changes :)

[43081j]

could you add some tests for this too?

just also wondered - won't there always be a statement parent somewhere, so it'll never be nullish?

tests will help prove it out though

[pndewit]

I've tried to add a test case, but it's quite hard as the AST will get more than a simple css expression as all the other tests are using.

The old code was handling the case that getStatementParent returns something nullish:

const statement = path.getStatementParent();

if (statement && statement.node.leadingComments) { ... }

So I left that handling intact by adding optional chaining.

[pndewit]

I now see I made a boo-boo, my change only enables this to work (note the array):

const MY_CONST = 600;

class MyClass {
  static styles = [
    // postcss-lit-disable-next-line
    css`
      @media (width >= ${MY_CONST}px) {
        .foo { color: hotpink; }
      }
    `,
  ];
}

But I also need this to work (note the absence of the array):

const MY_CONST = 600;

class MyClass {
  // postcss-lit-disable-next-line
  static styles = css`
    @media (width >= ${MY_CONST}px) {
      .foo { color: hotpink; }
    }
  `;
}

[pndewit]

I now see I made a boo-boo, my change only enables this to work (note the array):

const MY_CONST = 600;

class MyClass {
  static styles = [
    // postcss-lit-disable-next-line
    css`
      @media (width >= ${MY_CONST}px) {
        .foo { color: hotpink; }
      }
    `,
  ];
}

But I also need this to work (note the absence of the array):

const MY_CONST = 600;

class MyClass {
  // postcss-lit-disable-next-line
  static styles = css`
    @media (width >= ${MY_CONST}px) {
      .foo { color: hotpink; }
    }
  `;
}

I fixed this by also checking the parent node.

This also fixed the tests I tried to add:

1. should ignore a child node -> This one wasn't working before my latest pushes
2. should ignore a child node from an array -> This one was working before my latest pushes and continues to work
3. should ignore everything inside a disabled parent node -> This one was always working, even without my changes

[43081j]

It's a little too specific checking the parent and the current node I think

I had a quick dig and I think what we're looking for is findParent:

let parentStatement = undefined;
const disableNode = path.findParent((p) => {
  // Determine if this is the statement
  if (!parentStatement) {
    if (!p.parentPath || (Array.isArray(p.container) && p.isStatement()) {
      parentStatement = p;
    }
  } else {
    // We already saw the root statement, stop looking
    return false; 
  }
  // Check if this parent has a disable comment 
  if (hasDisableComment(p)) {
    return true; 
  }
  return false; 
});
if (disableNode) {
  // It is disabled
}

So this will support any parent node having a disabled comment until we reach the root statement

I wrote this on mobile so some names are probably wrong

[pndewit]

How about this? 😄

[coveralls]

Pull Request Test Coverage Report for Build 12376389881

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.08%) to 96.794%

---

Files with Coverage Reduction	New Missed Lines	%
lib/test/parse_test.js	1	99.51%
lib/util.js	2	96.12%

Totals
Change from base Build 8234101906:	0.08%
Covered Lines:	906
Relevant Lines:	914

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 12316190204

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.08%) to 96.794%

---

Files with Coverage Reduction	New Missed Lines	%
lib/test/parse_test.js	1	99.51%
lib/util.js	2	96.12%

Totals
Change from base Build 8234101906:	0.08%
Covered Lines:	906
Relevant Lines:	914

---

💛 - Coveralls