fix: make types usable in CommonJS#44
Conversation
| "@eslint/plugin-kit": "^0.2.3", | ||
| "@eslint/core": "^0.10.0", | ||
| "@eslint/plugin-kit": "^0.2.5", | ||
| "@types/css-tree": "^2.3.10", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 add these annotations to the imports in our code, or mention in the docs how to deal with CSSTree types in Deno. We don't provide specific instructions for Deno or jsr at the moment.
There was a problem hiding this comment.
I think it's fine to not worry about JSR or Deno until we have evidence anyone is using the packages from there. At that point, I'd imagine we'd have some repro cases to work with the Deno team on.
mdjermanovic
left a comment
There was a problem hiding this comment.
LGTM, thanks! Would like @nzakas to verify before merging.
|
Oh, and there's a merge conflict in package.json. |
|
Fixed the merge conflict. |
|
Converting to a draft because I need to fix the build in Bun. |
Prerequisites checklist
What is the purpose of this pull request?
Similar to eslint/rewrite#143 and eslint/json#77, this PR ensures that the
@eslint/csstypes can be imported from a CommonJS module whenmoduleResolutionis one of'node16'or'nodenext'. This is currently not possible because of an issue with the types in@eslint/plugin-kitthat has been fixed in the latest versionv0.2.5.What changes did you make? (Give an overview)
@eslint/plugin-kitto version spec"^0.2.5".@eslint/corea runtime dependency of this package.@types/eslintdependency which is no longer needed.@types/css-treefrom the localtypingsdirectory.@eslint/csscan be imported from a.ctsfile.Related Issues
refs eslint/rewrite#143, refs eslint/json#77.
Is there anything you'd like reviewers to focus on?