-
-
Notifications
You must be signed in to change notification settings - Fork 12
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
feat: Type checking for all APIs and rules #103
base: main
Are you sure you want to change the base?
Conversation
Probably in both cases Rollup thinks that the JSDoc comments under "Type Definitions" are part of the next statement, i.e. //-----------------------------------------------------------------------------
// Type Definitions
//-----------------------------------------------------------------------------
/**
* @import { AtrulePlain } from "@eslint/css-tree"
* @import { CSSRuleDefinition } from "../types.js"
* @typedef {"unknownAtRule" | "invalidPrelude" | "unknownDescriptor" | "invalidDescriptor" | "invalidExtraPrelude" | "missingPrelude"} NoInvalidAtRulesMessageIds
* @typedef {CSSRuleDefinition<{ RuleOptions: [], MessageIds: NoInvalidAtRulesMessageIds }>} NoInvalidAtRulesRuleDefinition
*/
//-----------------------------------------------------------------------------
// Rule Definition
//----------------------------------------------------------------------------- |
Ah, that is brilliant! Thanks! |
Tests are failing due to this: eslint/csstree#27 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CI build is now failing on the "Verify JSR Publish" job. It looks like "dist/esm/types.ts"
is missing in the list of included files in jsr.json
:
Lines 9 to 17 in 2519f4e
"include": [ | |
"dist/esm/index.js", | |
"dist/esm/index.d.ts", | |
"dist/esm/syntax/index.js", | |
"dist/esm/syntax/index.d.ts", | |
"README.md", | |
"jsr.json", | |
"LICENSE" | |
] |
Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 17 out of 19 changed files in this pull request and generated no comments.
Files not reviewed (2)
- jsr.json: Language not supported
- package.json: Language not supported
Comments suppressed due to low confidence (2)
src/rules/use-baseline.js:495
- The repeated inline conversion of baselineAvailability.availability using String() suggests an underlying type mismatch. Consider revising the type of baselineAvailability.availability upstream so that it already has the expected string type.
availability: baselineAvailability.availability,
rollup.config.js:25
- [nitpick] Please verify that using the ".cts" extension for the CommonJS type definitions aligns with project conventions and tooling expectations.
rename: "types.cts",
Co-authored-by: Francesco Trotta <[email protected]>
Co-authored-by: Francesco Trotta <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! I would like another review.
Prerequisites checklist
What is the purpose of this pull request?
Enable type checking for the plugin.
What changes did you make? (Give an overview)
@eslint/json
@eslint/css-tree
Related Issues
Is there anything you'd like reviewers to focus on?
Right now, Rollup is removing the type definitions for
no-invalid-at-rules
andno-invalid-properties
. I can't quite figure out why that is. I could use some help with this.