Conversation
|
It seems that the CI failure is unrelated to this change. |
|
I investigated this and it appears to be a regression in I tried addressing it but couldn't find a clear solution, so I opened an issue in the |
lumirlumir
force-pushed
the
fix-make-the-data-property-stricter
branch
from
1789094 to
440b8c2
Compare
|
Marking it as a draft until #332 is merged. |
|
This PR is now ready for re-review. |
There was a problem hiding this comment.
Pull request overview
This PR restricts the data property type in violation reports and suggestions from the overly permissive Record<string, unknown> to a new stricter type MessagePlaceholderData that only allows primitive types: string | number | boolean | bigint | null | undefined. This prevents non-serializable types (objects, arrays, functions, symbols, etc.) from being used in message placeholders, which is important since these values are typically converted to strings for display in error messages.
- Introduced
MessagePlaceholderDatatype to enforce strict typing on message placeholder data - Updated both
ViolationReportBaseandSuggestedEditBaseinterfaces to use the new type - Added comprehensive type tests to verify allowed primitives work correctly and disallowed types (objects, arrays, functions, symbols, maps, sets) are properly rejected at compile time
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/core/src/types.ts | Defines new MessagePlaceholderData type and updates data property types in ViolationReportBase and SuggestedEditBase interfaces |
| packages/core/tests/types/types.test.ts | Adds comprehensive test coverage for allowed primitive types and uses @ts-expect-error to verify disallowed complex types are properly rejected |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…com/eslint/rewrite into fix-make-the-data-property-stricter
nzakas
left a comment
nzakas
left a comment
There was a problem hiding this comment.
LGTM. Would like another review before merging.
github-project-automation
bot
moved this from Second Review Needed
to Complete
in Triage
…#18396) Hi, this is my first contribution here, so I'm not sure whether opening a PR directly is appropriate. If anything is wrong, please let me know! --- In this PR, I've corrected the `DiagnosticData` type to `Record<string, string | number | boolean | bigint | null | undefined>;` In ESLint v9.39.2 (which uses `@eslint/core@0.17.0`), this type was widened to `Record<string, unknown>`. Refs: - https://github.com/eslint/rewrite/releases/tag/core-v0.17.0 - eslint/rewrite#298 However, in the ESLint v10.0.0 pre-release (which uses `@eslint/core@1.0.1`), this type have been narrowed to `Record<string, string | number | boolean | bigint | null | undefined>;`. Refs: - https://github.com/eslint/rewrite/releases/tag/core-v1.0.1 - eslint/rewrite#327 If the type's intention is to ensure maximum compatibility with any ESLint version, then `Record<string, string | number | boolean | bigint | null | undefined>` seems the most appropriate choice here.
Prerequisites checklist
What is the purpose of this pull request?
In this PR, I've resolved the issue metioned in #310.
I've updated the
dataproperty to acceptstring | number | boolean | bigint | null | undefinedand added test cases to ensure it works.What changes did you make? (Give an overview)
In this PR, I've resolved the issue metioned in #310.
Related Issues
Closes: #310
Is there anything you'd like reviewers to focus on?
N/A