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

fix: make types usable in CommonJS #44

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

fix: make types usable in CommonJS #44

wants to merge 6 commits into from

Conversation

fasttime
Copy link
Member

@fasttime fasttime commented Jan 14, 2025

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/css types can be imported from a CommonJS module when moduleResolution is one of 'node16' or 'nodenext'. This is currently not possible because of an issue with the types in @eslint/plugin-kit that has been fixed in the latest version v0.2.5.

What changes did you make? (Give an overview)

  • Updated dependency @eslint/plugin-kit to version spec "^0.2.5".
  • Made @eslint/core a runtime dependency of this package.
  • Removed @types/eslint dependency which is no longer needed.
  • Installing @types/css-tree from the local typings directory.
  • Added tests to ensure that @eslint/css can be imported from a .cts file.

Related Issues

refs eslint/rewrite#143, refs eslint/json#77.

Is there anything you'd like reviewers to focus on?

package.json Outdated
"@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.

@fasttime fasttime marked this pull request as ready for review January 14, 2025 18:03
mdjermanovic
mdjermanovic previously approved these changes Jan 15, 2025
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Would like @nzakas to verify before merging.

@mdjermanovic
Copy link
Member

Oh, and there's a merge conflict in package.json.

@fasttime
Copy link
Member Author

Fixed the merge conflict.

mdjermanovic
mdjermanovic previously approved these changes Jan 15, 2025
@eslint-github-bot eslint-github-bot bot added the bug Something isn't working label Jan 20, 2025
@fasttime
Copy link
Member Author

Converting to a draft because I need to fix the build in Bun.

@fasttime fasttime marked this pull request as draft January 20, 2025 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Implementing
Development

Successfully merging this pull request may close these issues.

3 participants