Fix Could not find a declaration file for module 'css-tree' type error

[danielrentz]

Which issue, if any, is this issue related to?

Closes #8435

Is there anything in the PR that needs further explanation?

The types.d.ts published by stylelint imports from "css-tree", so their types definitions (from @types/css-tree) should be installed as well.

[changeset-bot]

🦋 Changeset detected

Latest commit: fe797b4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
stylelint	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[ybiquitous]

I'm wondering if a library like Stylelint should provide 3rd-party type packages (i.e. @type/*) because all users don't need types... 🤔

[danielrentz]

Well this discussion pops up everywhere :)

In my opinion ... A lot of npm packages already bring bundled type information, for example postcss (which Stylelint depends on) installs all its type information - although not all users need them. So css-tree and @types/css-tree should be seen as one "logical package" that should be installed by Stylelint if it depends on it publically, to allow seemless integration in TS based projects.

[ybiquitous]

If css-tree (not @types/css-tree) provided type definitions, I would have no objections. 😅

I'm still concerned whether we should provide all @types/* packages the Stylelint depends on... 🤔

stylelint/package.json

Lines 179 to 192 in a335c8b

	"@types/balanced-match": "^1.0.4",
	"@types/css-tree": "^2.3.10",
	"@types/debug": "^4.1.12",
	"@types/file-entry-cache": "^5.0.4",
	"@types/global-modules": "^2.0.2",
	"@types/globjoin": "^0.1.2",
	"@types/imurmurhash": "^0.1.4",
	"@types/micromatch": "^4.0.9",
	"@types/normalize-path": "^3.0.2",
	"@types/postcss-less": "^4.0.6",
	"@types/postcss-resolve-nested-selector": "^0.1.3",
	"@types/postcss-safe-parser": "^5.0.4",
	"@types/svg-tags": "^1.0.2",
	"@types/write-file-atomic": "^4.0.3",

And, for type-checking on Stylelint development, we have applied a patch to @types/css-tree. Would this patching raise any problems?
https://github.com/stylelint/stylelint/blob/a335c8b837155955da702fd842cb19bdd61c2063/patches/%40types%2Bcss-tree%2B2.3.10.patch

[romainmenke]

Maybe better to write specific types for the parts of the css-tree API that are exposed by Stylelint? In this case that would only be the syntax definition part, and not any other types like the lexer, tokenizer, ...

[ybiquitous]

Ideally, it'd be best that css-tree provide its types. It was discussed in the past, but the discussion is recently inactive.
csstree/csstree#88

@ryo-manba recently committed to @types/css-tree, so he may have some ideas:
https://github.com/DefinitelyTyped/DefinitelyTyped/commits/master/types/css-tree

@types/css-tree is also risky to become unmaintained, see below:
DefinitelyTyped/DefinitelyTyped#62536

In short, I think we'd all be happy if we could resolve the issue upstream. 😃

[ryo-manba]

Sorry, I should have addressed this when I added the type definitions.

Since we are only adding the Lexer type definition, I agree with Romain’s suggestion to define only what we need ourselves. If we need to expose more types in the future, we can reconsider it then.

By the way, eslint/css includes its own type definitions as dependencies (Reference: eslint/css#44).

[ybiquitous]

@romainmenke Not 100% sure, but do you think it's challenging to add types upstream (on the css-tree side) instead of the type publication by Stylelint? 🤔

I'm worrying if users might be confused about whether Stylelint would provide such css-tree types, e.g. "why does this type come from Stylelint, not css-tree?" etc.

[romainmenke]

do you think it's challenging to add types upstream (on the css-tree side) instead of the type publication by Stylelint?

I think this would be indeed challenging.
Adding TypeScript support in any project always seems to surface a lot of opinions and requires several choices to be made (port to TypeScript, JS Doc, manually written types)
I don't think we will suddenly see css-tree making large steps here.

---

I did however misunderstand why we have a public dependency on css-tree types.
I thought this was about the syntax definition, but instead it seems to be context.lexer.

I don't think we want to have our own copy of this type.
It is very complex.

With that in mind I don't really see a better option than moving @types/css-tree to our public dependencies.

Maybe we could create some more specialized utilities on top of the css-tree lexer and expose those instead of the entire lexer?

[github-actions]

Try the Instant Preview in Online Demo

Stylelint Online Demo

Install the Instant Preview to Your Local

npm i -D https://pkg.pr.new/stylelint@8452

---

View Commit

[ybiquitous]

@romainmenke Thank you for the explanation. Now I understand the difficulties of adding types to css-tree upstream and that adding this workaround of @types/css-tree as a public dependency is required for now. 👍🏼

To make this problem easier to see, I created a small reproduction gist based on the #8435 report:
https://gist.github.com/ybiquitous/b7746ba3581e1c285628fb51c244cb1b

Please run the following commands for the reproduction:

git clone https://gist.github.com/ybiquitous/b7746ba3581e1c285628fb51c244cb1b test
cd test
npm i && npx tsc

Then, the output should be:

...
Errors  Files
     2  node_modules/@nodelib/fs.scandir/out/providers/async.d.ts:1
     3  node_modules/@nodelib/fs.scandir/out/types/index.d.ts:1
     2  node_modules/@nodelib/fs.stat/out/adapters/fs.d.ts:1
     3  node_modules/@nodelib/fs.stat/out/types/index.d.ts:1
     2  node_modules/@nodelib/fs.walk/out/index.d.ts:1
     2  node_modules/@nodelib/fs.walk/out/readers/async.d.ts:1
     2  node_modules/@nodelib/fs.walk/out/types/index.d.ts:1
     2  node_modules/fast-glob/out/index.d.ts:1
     2  node_modules/fast-glob/out/types/index.d.ts:1
     1  node_modules/globby/index.d.ts:124
     1  node_modules/stylelint/types/stylelint/index.d.ts:4

The error is caused by our dependency on the Lexer type of css-tree, which comes from @types/css-tree actually:

stylelint/types/stylelint/index.d.ts

Line 4 in 1ec1253

import type { Lexer } from 'css-tree';

So, I agree with publishing this PR, and I'm sorry for taking time. 🙇🏼

---

But, we will have to be more careful so not to publish our patched types in the future: ⚠️

stylelint/patches/@types+css-tree+2.3.10.patch

Lines 1 to 18 in a335c8b

	diff --git a/node_modules/@types/css-tree/index.d.ts b/node_modules/@types/css-tree/index.d.ts
	index b9c1d73..f3d9ba9 100644
	--- a/node_modules/@types/css-tree/index.d.ts
	+++ b/node_modules/@types/css-tree/index.d.ts
	@@ -910,7 +910,12 @@ export class Lexer {
	export const lexer: Lexer;
	export function fork(extension: {
	- atrules?: Record<string, string> | undefined;
	+ atrules?: Record<
	+ string, {
	+ comment?: string | undefined;
	+ prelude?: string | undefined;
	+ descriptors?: Record<string, string> | undefined;
	+ }> | undefined;
	properties?: Record<string, string> | undefined;
	types?: Record<string, string> | undefined;
	cssWideKeywords?: Array<string> | undefined;

Luckily, the Lexer type has not been patched yet. 😅

[ybiquitous]

Thank you for the PR, LGTM 👍🏼

[ybiquitous]

[note] This is not a blocker. Please let me just express my opinion.

We should probably have published a Stylelint own wrapper type for the CSSTree Lexer type instead.

stylelint/docs/developer-guide/rules.md

Line 310 in 1ec1253

- `lexer`(object): The [CSSTree](https://github.com/csstree/csstree) lexer, which helps in parsing and validating CSS syntax dynamically. The lexer is initialized once per lint session and reused across rules.

stylelint/types/stylelint/index.d.ts

Line 374 in 1ec1253

lexer?: Lexer | undefined;

From a long-term perspective of Stylelint public API, it does not seem wise for me to depend on a specific third party. If we'd like to switch to an alternative, it would be more difficult since the community already depended on CSSTree.

Because of my recent busy days, I may lack an understanding of the background around the Lexer type changes, sorry. 🙇🏼

[romainmenke]

I agree and it is unfortunate that we overlooked this when implementing the language options.

But I think it is not too late to change this, even when it technically is a breaking change. Chances are very slim that any 3rd party rule already depends on this.

We could first make this an internal type, remove the docs for context.lexer and then see how we proceed from there?
Then we can still continue using the language options in core rules.

Something like:

	/**
	 * A rule context.
	 */
	export type RuleContext = {
		configurationComment?: string | undefined;
		fix?: boolean | undefined;
		newline?: string | undefined;
		/** @internal */
		lexer?: unknown | undefined;
	};

And then cast to Lexer in getLexer :

/** @import { Lexer } from 'css-tree' */

/**
 * @param {import('stylelint').RuleContext} context
 * @returns {Lexer}
 */
export default function getLexer(context) {
	if (!context?.lexer) {
		throw new Error('Expected a "lexer" object');
	}

	if (!('matchAtrulePrelude' in Object(context?.lexer))) {
		throw new Error('Expected lexer" to be a css-tree lexer');
	}

	const lexer = /** @type Lexer */ (context.lexer);

	return lexer;
}

[ybiquitous]

I see. Honestly, I'm not sure about the impact on the user land (e.g., plugins), but this might be the last opportunity to reconsider the lexer property. The idea of marking lexer as internal sounds good to me if we can ignore the breaking change. 👍🏼

Reconsidering the published context.lexer property.

[jeddy3]

It sounds like a good way forward, and it's not too late to make this change.

@danielrentz Can you please update the PR to:

• mark the type as internal
• remove context.lexer from developer-guide docs
• cast to Lexer in getLexer()

I can make time to do a release once it's done.

[danielrentz]

@jeddy3 @romainmenke done.

[ybiquitous]

We should revert the dependency change, right?

[danielrentz]

@ybiquitous I did that in my 2nd commit

[ybiquitous]

Oh I missed the revert, sorry. LGTM except for the next version topic, minor or patch.

[ybiquitous]

Should this PR be considered as minor instead of patch because it makes a change of the part of the public API?

[jeddy3]

@danielrentz Thanks for making the changes. LGTM.

@ybiquitous Thanks for looking over it.

I'll merge and release this afternoon.