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

Add resolver to configuration #4088

Open
sindresorhus opened this issue May 25, 2019 · 18 comments
Open

Add resolver to configuration #4088

sindresorhus opened this issue May 25, 2019 · 18 comments
Labels
help wanted is likely non-trival and help is wanted status: ask to implement ask before implementing as may no longer be relevant type: enhancement a new feature that isn't related to rules

Comments

@sindresorhus
Copy link

What is the problem you're trying to solve?

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 annoying stylelint-disable comments, which eventually just leads to disabling those rules all together.

What solution would you like to see?

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.

@alexander-akait
Copy link
Member

I think it is not easy due difference resolution logic

@hudochenkov
Copy link
Member

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 believe it's possible to create a plugin, which will do imports by postcss-import and then check no-unknown-animations using stylelint.utils.checkAgainstRule util.

I don't think this issue would be solved in stylelint itself.

@hudochenkov hudochenkov added the status: needs discussion triage needs further discussion label Jun 10, 2019
@sindresorhus
Copy link
Author

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.

@alexander-akait
Copy link
Member

alexander-akait commented Jun 11, 2019

In theory we can implement resolver function in configuration, example eslint-plugin-import support same for node and webpack, maybe we can do same

@jeddy3 jeddy3 changed the title Support linter rules working across files Add resolver to configuration Jan 19, 2022
@jeddy3 jeddy3 added status: ready to implement is ready to be worked on by someone type: enhancement a new feature that isn't related to rules and removed status: needs discussion triage needs further discussion labels Jan 19, 2022
@jeddy3 jeddy3 pinned this issue Sep 26, 2022
@jeddy3
Copy link
Member

jeddy3 commented Sep 27, 2022

This feature would benefit these two existing rules:

And these two proposed rules:

Discussion in #6361.

@silverwind
Copy link

silverwind commented Apr 2, 2023

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.

@Mouvedia
Copy link
Contributor

@jeddy3 should we add the priority: high label?

@jeddy3
Copy link
Member

jeddy3 commented Jun 26, 2023

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.

@romainmenke
Copy link
Member

romainmenke commented Dec 11, 2023

In theory we can implement resolver function in configuration, example eslint-plugin-import support same for node and webpack, maybe we can do same

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.
In CSS you need to also do the inverse lookup and check which files import the current file.


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.

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)

postcss-import is progressing a bit slowly, but it's almost there.
I don't know what will happen with lightningcss as it is the only bundler that isn't fixing issues.

If bundlers work the same it is also easier to have good tooling.
You could let users of Stylelint define "entrypoints" for bundles and Stylelint could then build a dependency graph and check which bundles the currently open stylesheet is a part of.
All without having to know which bundler will be used.

  • foo.css is the open file
  • style.css has @import 'foo.css';
  • style.css is defined as an entrypoint
  • Stylelint detects that style.css and all it's imports are relevant because it is an entrypoint and because it's graph contains foo.css.

CSS-in-JS is not compatible with such an approach.


An alternative approach could mirror @csstools/postcss-global-data

This plugin injects CSS and then removes it again later.

This allows users to inject custom properties, custom media, ...
Other plugins can then use that information for transpilation.

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.

@ybiquitous
Copy link
Member

@romainmenke Thanks for the info. At the first point, I prefer the @csstools/postcss-global-data approach (see parseImport()). This seems limited to a local file system, but I believe it can cover most cases.

@jeddy3
Copy link
Member

jeddy3 commented Dec 11, 2023

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!

Stylelint could equally allow users to define files that must always be injected before the contents of the current file.

Do we think this should be per rule (like importFrom) or a global config?

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?)

@romainmenke
Copy link
Member

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 root nodes that any rule can consume to build up state.

Is this something that could be added to RuleContext ?

/**
 * 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 */ });

Do we think this should be per rule (like importFrom) or a global config?

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.


And do we envisage it supporting custom syntaxes?

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.

@ybiquitous
Copy link
Member

Adding a global config for imports and adding roots to a rule context sounds good. 👍🏼

@jeddy3
Copy link
Member

jeddy3 commented Dec 12, 2023

It sounds very promising.

To summarise:

  • a global config property (that supports custom syntaxes) to populate injectedRoots
  • applicable rules, e.g. no-unknown-animations, use injectedRoots by default
  • an optional secondary option on rules so that users can customise what roots are used (if any)

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.

@ybiquitous
Copy link
Member

Sounds good. A PoC will help us understand what we actually need. 👍

Copy link
Contributor

This issue is older than one month. Please ask before opening a pull request, as it may no longer be relevant.

@github-actions github-actions bot added status: ask to implement ask before implementing as may no longer be relevant and removed status: ready to implement is ready to be worked on by someone labels Jan 22, 2024
@EdwardHinkle
Copy link

Hi, just checking in. Is someone actually working on a PoC for this?

@romainmenke
Copy link
Member

I am not actively working on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted is likely non-trival and help is wanted status: ask to implement ask before implementing as may no longer be relevant type: enhancement a new feature that isn't related to rules
Development

No branches or pull requests

9 participants