-
-
Notifications
You must be signed in to change notification settings - Fork 929
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
Add resolver to configuration #4088
Comments
It has too many false-positives: - refined-github/refined-github#2070 - stylelint/stylelint#4088
I think it is not easy due difference resolution logic |
Thanks for the report and for using the template. I think it's bundlers territory to make dependency graphs. Or dedicated plugins. One of solutions I described in the mentioned issue:
I don't think this issue would be solved in stylelint itself. |
That's fair. It's not an easy problem to solve. But we all know if Stylelint doesn't handle this itself, it's not going to be handled. Proposing hacks is just a nice way to decline. I use Stylelint a lot and just want to see its rules become better. There are just so many rules that will stay inherently faulty because of lack of import graph knowledge, or new rules that will be impossible to do because of this. |
In theory we can implement |
This feature would benefit these existing rules:
see also #6746 |
This may be breaking to editor integrations that only pass single files to stylelint. Note, I havent't checked those integrations in detail, and I do know that multi-file rules like eslint-plugin-import do work in SublimeLint, so I assume that plugin may do something special to work in single file context too. |
@jeddy3 should we add the priority: high label? |
We typically reserve that label for bugs that impact a lot of people, i.e. something that'd trigger an immediate release. The issue is pinned for visibility, which I think is enough. |
I don't think we can draw a parallel to JavaScript imports because these are very different from bundled stylesheets. For JavaScript it is sufficient to look at the imports of the current file.
I've recently been spending a lot of time to try and align multiple bundlers around a shared understanding of how CSS must be bundled. (https://github.com/romainmenke/css-import-tests)
If bundlers work the same it is also easier to have good tooling.
CSS-in-JS is not compatible with such an approach. An alternative approach could mirror This plugin injects CSS and then removes it again later. This allows users to inject custom properties, custom media, ... Stylelint could equally allow users to define files that must always be injected before the contents of the current file. This works well enough in almost all cases and it is agnostic of how stylesheets will be bundled and/or combined in a document. |
@romainmenke Thanks for the info. At the first point, I prefer the |
Yes, the second approach looks promising. @romainmenke It's a boon having your input and involvement in Stylelint. You have a wealth of knowledge about the CSS specifications (e.g. #7359 (comment)) and an in-depth understanding of PostCSS (and other CSS tooling). Your contributions are greatly appreciated!
Do we think this should be per rule (like And do we envisage it supporting custom syntaxes? For example: {
"imports": [
{
"files": "custom-properties.css"
}, {
"files": "**/*.scss"
"customSyntax": "postcss-scss"
}
],
"rules": {
"no-unknown-animations": true
}
} (As an aside, it's interesting to see your simplification of tooling when migrating that package to stylelint@16. I hope the migration process was OK. Is there anything you think we should add to the migration guide to help others?) |
A side effect of injecting extra CSS into the same stylesheet is that the injected CSS will also be linted and might generate errors. This is not ideal when the injected CSS is third party code. (e.g. a library of design tokens like Open Props). It also complicates rules that fix source code as we do not want the injected CSS to actually be part of the output when writing fixes back to the source file. So I think the injected CSS should instead be exposed as a list of extra Is this something that could be added to /**
* A rule context.
*/
export type RuleContext = {
configurationComment?: string | undefined;
fix?: boolean | undefined;
newline?: string | undefined;
injectedRoots?: Array<{name: string, root: PostCSS.Root}>; /* needs a proper name */
}; Helpers to walk all nodes from an arbitrary set of roots might be nice. rootList(root, context).walkAtRules((x) => { /* add to state of rule */ });
I am leaning towards global config. But maybe it needs to conditional per linted file? So that users can have different sets of injected stylesheet depending on the file that is currently being processed.
I think that this should be supported because plugins for custom syntaxes should be able to use the same mechanics and not build their own. Mixing different syntaxes in a single config might not give good results, but that is to be expected. |
Adding a global config for imports and adding roots to a rule context sounds good. 👍🏼 |
It sounds very promising. To summarise:
Does that sound right? We'll probably need to flesh out that latter one, but I think there's enough to go on here to rustle up a PoC. |
Sounds good. A PoC will help us understand what we actually need. 👍 |
This issue is older than one month. Please ask before opening a pull request, as it may no longer be relevant. |
Hi, just checking in. Is someone actually working on a PoC for this? |
I am not actively working on this. |
Some rules needs to know about all the imported files to work effectively. For example, the
no-unknown-animations
(Related issue #2363) rule is not very useful as it currently only works for animations defined in the same file, but it's common to define animations in a separate file. This leads to a lot of annoyingstylelint-disable
comments, which eventually just leads to disabling those rules all together.It would be great if the linter created a graph of all the files being linted, including imported ones, and rules could use that information.
The text was updated successfully, but these errors were encountered: