Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Rule: use-layers #18

Open
1 task
mayank99 opened this issue Nov 27, 2024 · 12 comments · May be fixed by #27
Open
1 task

New Rule: use-layers #18

mayank99 opened this issue Nov 27, 2024 · 12 comments · May be fixed by #27
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion feature

Comments

@mayank99
Copy link

mayank99 commented Nov 27, 2024

Rule details

Flag any CSS rules that are outside a @layer block.

What type of rule is this?

Warns about a potential problem

Example code

This will be flagged:

/* ❌ */

p {
  color: red;
}

@media (width > 500px) {
  .box {
    padding: 12px;
  }
}

This is fine:

/* ✅ */

@layer foo {
  p {
    color: red;
  }
}

@media (width > 500px) {
  @layer bar {
    .box {
      @layer baz {
        padding: 12px;
      }
    }
  }
}

Note: Nested layers can used as @layer foo.bar {…} and anonymous layers can be used as @layer {…}. Both of these are fine and should not be flagged.

Some CSS, like @import, is fine to keep outside a @layer, because the contents of the file would still be using @layer.

@import "foo.css";
@import "bootstrap.css" layer(thirdparty);

@layer foo {…}

Participation

  • I am willing to submit a pull request to implement this rule.

Additional comments

When working with cascade layers, unlayered styles are usually a mistake. It is easy to forget to wrap the styles in @layer, which is exactly the kind of thing a linter should help with.

This rule probably should be opt-in, because cascade layers aren't being widely used yet.

@nzakas
Copy link
Member

nzakas commented Nov 27, 2024

I'm not familiar with cascade layers. Are you saying that if there's one @layer in a file then everything should be contained in an @layer? More detail would be appreciated.

@mayank99
Copy link
Author

mayank99 commented Nov 27, 2024

Thanks for considering this feature. Happy to explain in more detail.

The main problem here is that unlayered styles will take precedence over layered styles. When using layers in a codebase, we usually want everything to be in some layer. (The file doesn't matter, it's more about the overall codebase)

Here's an example scenario:

  1. You might decide that your project will be set up using three cascade layers: @layer globals, components, utilities. So, within your CSS files, you will want to use one of these layers.
  2. Let's say one day you are adding a new component to the project and writing some CSS for it. This CSS should go in @layer components.
  3. If you forget to wrap the component CSS in @layer components, then it will incorrectly take precedence over the utilities layer. Eslint can help prevent this situation and maintain code discipline.

One possible complication is that some people might prefer to write unlayered CSS and import it into a layer. I'm not sure if it's possible for eslint to detect that across files.

This kind of usage should be ok:

@import "unlayered-styles.css" layer(some-layer);

As a workaround, users could exclude this eslint rule from being applied on certain files/directories.


A related problem that should probably be a separate issue is that it's too easy to misspell a layer name, e.g. @layer utilitiies, which can break everything and cause confusion. Maybe that needs a custom eslint plugin though.

@fasttime fasttime added this to Triage Nov 28, 2024
@github-project-automation github-project-automation bot moved this to Needs Triage in Triage Nov 28, 2024
@nzakas
Copy link
Member

nzakas commented Dec 2, 2024

Got it, thanks. ESLint doesn't work across files, so we only know what's happening in the current file.

I can see how this would be valuable, as you probably don't want to mix layered and unlayered styles. And I agree, this shouldn't be enabled by default.

So it sounds like we'd just need to flag any rule that isn't inside of a layer?

@jeddy3
Copy link

jeddy3 commented Dec 2, 2024

I've seen a few different conventions for layers, including intentionally mixing layered and unlayered styles. Perhaps no-restricted-syntax is a good fit for people to craft something that enforces their preferred ones?

For example, to enforce that:

  • all styles are authored as unlayered
  • all imports are put into layers
rules: {
  "no-restricted-syntax": [
    "error",
    {
      selector: "Atrule[name=layer]",
      message: "The `@layer` at-rule is disallowed. Author unlayered styles instead.",
    },
    {
      selector: "Atrule[name=import]:not(:has(Layer))",
      message: "Imports must be put into a layer.",
    },
  }
}

Which would enforce this convention:

/* button.css */
.button {
  color: red;
}
/* styles.css */
@import "bootstrap.css" layer(vendor);
@import "defaults.css" layer(defaults);
@import "button.css" layer(components);
@import "slider.css" layer(components);

Or, to enforce the original use case that rules must be within a @layer:

{
  selector:
    ":matches(StyleSheet > Rule, StyleSheet > Atrule[name!=layer]:has(Rule):not(:has(Atrule[name=layer] Rule)))",
  message: "StyleSheet cannot contain unlayered rules",
},

(i.e. flag top-level rules and flag non-layer at-rules which contain rules outside of @layer. There may be a more concise selector...)

Which would enforce:

@layer foo {
  p {
    color: red;
  }
}

@media (width > 500px) {
  @layer bar {
    .box {
      @layer baz {
        padding: 12px;
      }
    }
  }
}

People can enforce other @layer conventions, too. Such as:

Disallowing anonymous layers:

{
  selector: "Atrule[name=layer]:not(:has(> AtRulePrelude))",
  message: "Anonymous layers are disallowed.",
},
@layer {
  body {}
}

And restricting the name of layers:

{
  selector: "Layer[name!=/\\b(utilities|components)\\b/]",
  message: "Only `utilities` and `components` layers are allowed.",
},
@layer unknown {
  body {}
}

There are likely more cascading layers conventions people would want to enforce. I'm unfamiliar with ESLint's conventions; when do things become rules, and when are they left to no-restricted-syntax?

@nzakas
Copy link
Member

nzakas commented Dec 2, 2024

@jeddy3 there are downsides of using no-restricted-syntax, the biggest being that it's not easy to modify just one entry in a config that is inheriting from another. For instance, imagine there's a base config that looks like this:

export default [{
rules: {
  "no-restricted-syntax": [
    "error",
    {
      selector: "Atrule[name=layer]",
      message: "The `@layer` at-rule is disallowed. Author unlayered styles instead.",
    },
    {
      selector: "Atrule[name=import]:not(:has(Layer))",
      message: "Imports must be put into a layer.",
    },
  }
}
}];

And you'd like to add a custom warning in your config file that inherits from that base config. You'll now need to pull out this definition of no-restricted-syntax using JavaScript, create a copy of the array, then push your new check onto the end of that array, before inserting into your new config.

Also note that the esquery syntax, while handy, does come with parsing overhead. The longer your selector, the slower the match will be. For any sufficiently long esquery string, it's really a good idea to just create a rule and implement that check in JavaScript.

So no-restricted-syntax is really a utility to use when you just want to add a quick rule without the overhead of another file.

When there are well-established patterns that would benefit multiple people, we generally look to creating a new rule with configurable options to cover 80-90% of cases. This has the benefit of a rule explicitly saying what it does (no-invalid-properties vs no-restricted-syntax). I think this use case, defining that layers are preferred in some way, definitely speaks to having its own rule.

A while back I created a utility to easily create new rules using no-restricted-syntax-style configuration:
https://github.com/humanwhocodes/eslint-simple-rule


Let me now if this sounds like a fairly cohesive description for this rule:

  1. Expect all rules to occur in named layers
  2. By default, don't require @import statements to have layer.
  3. Option: requireImportLayer: true to enable checking that @import statements have layer.
  4. Option: allowAnonymousLayers: true to allow layers without names.
  5. Option: layerNamesPattern that accepts a regex string that layer names must match (allowAnonymousLayers must be false for this).

Of course, if people want to mix layered and non-layered rules, they wouldn't enable this rule.

@mayank99
Copy link
Author

mayank99 commented Dec 3, 2024

It looks good, except maybe for this parenthetical at the end:

layerNamesPattern that accepts a regex string that layer names must match (allowAnonymousLayers must be false for this).

Sometimes it's fine to have an anonymous layer inside a named layer, so layerNamesPattern and allowAnonymousLayers don't need to be mutually exclusive. layerNamesPattern should only be concerned with disallowing unknown names.

@nzakas
Copy link
Member

nzakas commented Dec 3, 2024

Sometimes it's fine to have an anonymous layer inside a named layer, so layerNamesPattern and allowAnonymousLayers don't need to be mutually exclusive. layerNamesPattern should only be concerned with disallowing unknown names.

Good call out, I agree. 👍

I think I'd also probably call this use-layers because we actually want to enforce the use of layers rather than disallowing something.

@nzakas nzakas changed the title New Rule: no-unlayered-styles New Rule: use-layers Dec 3, 2024
@nzakas nzakas self-assigned this Dec 3, 2024
@nzakas nzakas moved this from Needs Triage to Ready to Implement in Triage Dec 3, 2024
@jeddy3
Copy link

jeddy3 commented Dec 4, 2024

Thank you for taking the time to explain the downsides of no-restricted-syntax and the ESLint conventions around rules. The eslint-simple-rule utility looks very useful!

Expect all rules to occur in named layers

If someone uses cascade layers via @import with layer(), I think they'd expect all rules to occur outside of @layer.

For example:

/* defaults.css */
a {
 color: pink;
}

button {
  color: red;

  &:hover {
    color: orange;
  }
}
/* primary-button.css */
.primary-button {
  color: blue;
}
/* secondary-button.css */
.seconday-button {
  color: green;
}
/* styles.css */
@import "defaults.css" layer(defaults);
@import "primary-button.css" layer(components);
@import "secondary-button.css" layer(components);

I think people take this approach so that:

  • the orchestration of layers occurs in one place
  • a developer working on a component doesn't need to work with layers in their component (other than knowing their styles will take precedence over any defaults regardless of specificity)

Can the use-layers rule support this use case, too?

I'm not sure the requireImportLayer: true option makes sense if someone uses cascade layers via @layer. I suspect people would want to enforce cascade layers via @layer or via @import with layer(), but not both.

Would moving the requireImportLayer option to be a primary one that switches modes resolve this?

For example, "use-layers": [error, "via-at-layer"] would:

  • Expect all rules to occur in named layers

And something like "use-layers": [error, "via-at-import"] would:

  • Expect all rules to occur outside of named layers
  • Expect all @import statements to use the layer() function

The allowAnonymousLayers and layerNamesPattern options would apply to both modes.

@mayank99
Copy link
Author

mayank99 commented Dec 4, 2024

I suspect people would want to enforce cascade layers via @layer or via @import with layer(), but not both.

Personally, I do use both. My setup involves @importing files into a top-level layer, and using @layer inside those files for nested layers.

@jeddy3
Copy link

jeddy3 commented Dec 4, 2024

That's fascinating. I did wonder if people used both approaches to nest their layers.

Is there a way to wrangle the options so that the rule supports both use cases?:

  • only using @import (via the layer() function) to layer styles
  • using @layer to layer styles (and optionally nest layers using @import)

I wonder if there are more use cases, too. It feels like there aren't established patterns around cascade layers yet.

@nzakas
Copy link
Member

nzakas commented Dec 4, 2024

Keep in mind that ESLint functions on only one file at a time. If you're using the pattern where all of your files don't contain layers except for your top-level file that imports everything into layers, then you'd only enable use-layer for that one file that's importing everything. There's no reason to enable it for other files. So you might do something like this:

import css from "@eslint/css";

export default [
    {
        plugins: {
            css
        },
        files: ["styles/*.css"],
        ...css.configs.recommended
    },
    {
        plugins: {
            css
        },
        files: ["styles/index.css"],
        rules: {
            "css/use-layers": "error"
        }
    }

];

@nzakas nzakas linked a pull request Dec 4, 2024 that will close this issue
1 task
@jeddy3
Copy link

jeddy3 commented Dec 4, 2024

I forgot about overrides, sorry 😅

Would it make sense to tweak the description to:

  • By default, require all rules to occur in named layers (e.g. @layer foo {})
  • By default, require @import statements to have named layers (e.g. @import "bar.css" layer(foo);)
  • Option: allowAnonymousLayers: true to allow layers without names (either @layer {} or @import "foo.css" layer;)
  • Option: layerNamesPattern that accepts a regex string that layer names must match

So that @mayank99's use case is enabled without options:

import css from "@eslint/css";

export default [
    {
        plugins: {
            css
        },
        files: ["styles/*.css"],
        ...css.configs.recommended
    },
    {
        rules: {
            "css/use-layers": "error"
        }
    }
];

i.e. use cascade layers everywhere (both to wrap rules and when importing).

So is the @import-only use case:

import css from "@eslint/css";

export default [
    {
        plugins: {
            css
        },
        files: ["styles/*.css"],
        ...css.configs.recommended
    },
    {
        plugins: {
            css
        },
        files: ["styles/index.css"],
        rules: {
            "css/use-layers": "error"
        }
    }
];

And the same for an @layer but no @import use case:

import css from "@eslint/css";

export default [
    {
        plugins: {
            css
        },
        files: ["styles/*.css"],
        ...css.configs.recommended
    },
    {
        plugins: {
            css
        },
        files: ["styles/components/**/.css"],
        rules: {
            "css/use-layers": "error"
        }
    }
];

@nzakas nzakas moved this from Ready to Implement to Implementing in Triage Dec 4, 2024
@fasttime fasttime added the accepted There is consensus among the team that this change meets the criteria for inclusion label Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion feature
Projects
Status: Implementing
Development

Successfully merging a pull request may close this issue.

4 participants