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.

Sign up for GitHub

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

fix: make types usable in CommonJS #44

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
+25 −7
Draft

fix: make types usable in CommonJS #44

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ade2a29
fix: make types usable in CommonJS
fasttime Jan 14, 2025
f489f6b
Merge branch 'main' into fix-cjs-types
fasttime Jan 15, 2025
9c3c05a
use local `@types/css-tree` package
fasttime Jan 20, 2025
89c1a09
don't use `node_modules/@types/css-tree` in development
fasttime Jan 22, 2025
2a8cb3a
use `node_modules` types instead of local typings
fasttime Jan 22, 2025
89d90c0
remove outdated `@types/eslint`
fasttime Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,12 @@
],
"license": "Apache-2.0",
"dependencies": {
"@eslint/plugin-kit": "^0.2.3",
"@eslint/core": "^0.10.0",
"@eslint/plugin-kit": "^0.2.5",
"@types/css-tree": "^2.3.10",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The generated dist files dist/cjs/index.d.cts and dist/esm/index.d.ts contain type imports from css-tree, for example:

export type CssNode = import("css-tree").CssNode;

css-tree does not contain built-in types, so it seems good to add a dependency on the types package. If we don't want to add a runtime dependency I think we should at least keep @types/css-tree in the devDependencies so that the types test can validate our own types against the css-tree types.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already have my own types for css-tree here:
https://github.com/eslint/css/blob/main/typings/css-tree.d.ts

(The @types/css-tree package is very out of date and was causing errors, so I created my own.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see now. What do you think about copying the typings directory to dist/typings and changing the type imports in TSDoc to use local paths instead of the package name, e.g. import("../typings/css-tree") instead of import("css-tree")? I think this would be the solution with the best compatibility for both npm and jsr if we don't want users to install the css-tree types by themselves.

Note: css-tree.d.ts would have to be minimally changed by removing the module declaration declare module "css-tree" { ... }. I'm not sure if there's another way, but I can check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems messy to me. I'd then need to manually apply all the types to the CSSTree API to get intellisense and type checking working.

This must already be a solved problem in the TypeScript ecosystem?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this approach type checking will work without having to apply the CSSTree types manually to JavaScript files - as long as the TSDoc comments are modified. I'm not sure though about intellisense.

Another solution is packaging the updated version of @types/css-tree as a local directory, like typings/css-tree/. Instead of the npm version, package.json could specify to install the contents of the local directory with "@types/css-tree": "file:./typings/css-tree". This will not require changing the sources and it should work well for npm as well. I'll just need to check how to get the same settings with jsr since jsr doesn't use package.json.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is, when I do something like import { parse } from "css-tree", then I won't get any type checking or intellisense for parse() if we remove the declare module "css-tree" from the type definitions.

The package.json approach seems like a better option to me. I'm not too worried about JSR at this point, as I'm not sure there's much usage there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I got everything working now. Apart from installing "@types/css-tree" from the local typings directory I changed tsconfig.json to avoid a namespace conflict in Bun, and it looks like intellisense is working fine in VSCode.

As for jsr, it seems that there is no standard to install a local types only package: they expect packages to include their own types. So the type mappings must be done directly by the runtime. For example in Deno using the @ts-types annotation. I'm not sure if we should mention this in the docs since we don't provide specific instructions for Deno or jsr at the moment.

"css-tree": "^3.0.1"
},
"devDependencies": {
"@eslint/core": "^0.7.0",
"@eslint/json": "^0.5.0",
"@types/eslint": "^8.56.10",
"c8": "^9.1.0",
Expand Down
10 changes: 10 additions & 0 deletions tests/types/cjs-import.test.cts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/**
* @fileoverview CommonJS type import test for ESLint CSS Language Plugin.
* @author Francesco Trotta
*/

//-----------------------------------------------------------------------------
// Imports
//-----------------------------------------------------------------------------

import "@eslint/css";
3 changes: 2 additions & 1 deletion tests/types/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,6 @@
"rootDir": "../..",
"strict": true
},
"files": ["../../dist/esm/index.d.ts", "types.test.ts"]
"files": [],
"include": [".", "../../dist"]
}
Loading