fix: Use updated types from @eslint/core #66
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,128 @@ | ||||||
| /** | ||||||
| * @fileoverview Additional types for this package. | ||||||
| * @author Nicholas C. Zakas | ||||||
| */ | ||||||
|
|
||||||
| //------------------------------------------------------------------------------ | ||||||
| // Imports | ||||||
| //------------------------------------------------------------------------------ | ||||||
|
|
||||||
| import type { | ||||||
| RuleVisitor, | ||||||
| TextSourceCode, | ||||||
| Language, | ||||||
| LanguageOptions, | ||||||
| RuleDefinition, | ||||||
| } from "@eslint/core"; | ||||||
|
Comment on lines
+10
to
+16
There was a problem hiding this comment.
There was a problem hiding this comment. At this point I honestly don't understand when types are supposed to be a dev dependency vs. a runtime dependency. What would you do? There was a problem hiding this comment. If references to It does feel weird making a types-only package be a runtime dependency. But 🤷 without first-party support from package managers on delineating "runtime" vs. "types-only" dependencies, it's all "runtime". |
||||||
| import { | ||||||
| DocumentNode, | ||||||
| MemberNode, | ||||||
| ElementNode, | ||||||
| ObjectNode, | ||||||
| ArrayNode, | ||||||
| StringNode, | ||||||
| NullNode, | ||||||
| NumberNode, | ||||||
| BooleanNode, | ||||||
| NaNNode, | ||||||
| InfinityNode, | ||||||
| IdentifierNode, | ||||||
| AnyNode, | ||||||
| Token, | ||||||
| } from "@humanwhocodes/momoa"; | ||||||
|
|
||||||
| //------------------------------------------------------------------------------ | ||||||
| // Types | ||||||
| //------------------------------------------------------------------------------ | ||||||
|
|
||||||
| type ValueNodeParent = DocumentNode | MemberNode | ElementNode; | ||||||
|
|
||||||
| /** | ||||||
| * A JSON syntax element, including nodes and tokens. | ||||||
| */ | ||||||
| export type JSONSyntaxElement = Token | AnyNode; | ||||||
|
|
||||||
| /** | ||||||
| * Language options provided for JSON files. | ||||||
| */ | ||||||
| export interface JSONLanguageOptions extends LanguageOptions { | ||||||
| /** | ||||||
| * Whether to allow trailing commas. Only valid in JSONC. | ||||||
| */ | ||||||
| allowTrailingCommas?: boolean; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * The visitor format returned from rules in this package. | ||||||
| */ | ||||||
| export interface JSONRuleVisitor extends RuleVisitor { | ||||||
| Document?(node: DocumentNode): void; | ||||||
| Member?(node: MemberNode, parent?: ObjectNode): void; | ||||||
| Element?(node: ElementNode, parent?: ArrayNode): void; | ||||||
| Object?(node: ObjectNode, parent?: ValueNodeParent): void; | ||||||
| Array?(node: ArrayNode, parent?: ValueNodeParent): void; | ||||||
| String?(node: StringNode, parent?: ValueNodeParent): void; | ||||||
| Null?(node: NullNode, parent?: ValueNodeParent): void; | ||||||
| Number?(node: NumberNode, parent?: ValueNodeParent): void; | ||||||
| Boolean?(node: BooleanNode, parent?: ValueNodeParent): void; | ||||||
| NaN?(node: NaNNode, parent?: ValueNodeParent): void; | ||||||
| Infinity?(node: InfinityNode, parent?: ValueNodeParent): void; | ||||||
| Identifier?(node: IdentifierNode, parent?: ValueNodeParent): void; | ||||||
|
|
||||||
| "Document:exit"?(node: DocumentNode): void; | ||||||
| "Member:exit"?(node: MemberNode, parent?: ObjectNode): void; | ||||||
| "Element:exit"?(node: ElementNode, parent?: ArrayNode): void; | ||||||
| "Object:exit"?(node: ObjectNode, parent?: ValueNodeParent): void; | ||||||
| "Array:exit"?(node: ArrayNode, parent?: ValueNodeParent): void; | ||||||
| "String:exit"?(node: StringNode, parent?: ValueNodeParent): void; | ||||||
| "Null:exit"?(node: NullNode, parent?: ValueNodeParent): void; | ||||||
| "Number:exit"?(node: NumberNode, parent?: ValueNodeParent): void; | ||||||
| "Boolean:exit"?(node: BooleanNode, parent?: ValueNodeParent): void; | ||||||
| "NaN:exit"?(node: NaNNode, parent?: ValueNodeParent): void; | ||||||
| "Infinity:exit"?(node: InfinityNode, parent?: ValueNodeParent): void; | ||||||
| "Identifier:exit"?(node: IdentifierNode, parent?: ValueNodeParent): void; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * The `SourceCode` implementation for JSON files. | ||||||
| */ | ||||||
| export interface IJSONSourceCode | ||||||
|
There was a problem hiding this comment. [Style] Was the
Suggested change
Here and with Aside: I've always found this discussion around the choice to not preserve that naming in TypeScript ... interesting: microsoft/TypeScript-Handbook#121 |
||||||
| extends TextSourceCode<{ | ||||||
| LangOptions: JSONLanguageOptions; | ||||||
| RootNode: DocumentNode; | ||||||
| SyntaxElementWithLoc: JSONSyntaxElement; | ||||||
| ConfigNode: Token; | ||||||
| }> { | ||||||
| /** | ||||||
| * Get the text of a syntax element. | ||||||
| * @param syntaxElement The syntax element to get the text of. | ||||||
| * @param beforeCount The number of characters to include before the syntax element. | ||||||
| * @param afterCount The number of characters to include after the syntax element. | ||||||
| * @returns The text of the syntax element. | ||||||
| */ | ||||||
| getText( | ||||||
| syntaxElement: JSONSyntaxElement, | ||||||
| beforeCount?: number, | ||||||
| afterCount?: number, | ||||||
| ): string; | ||||||
| } | ||||||
|
|
||||||
| export type IJSONLanguage = Language<{ | ||||||
|
There was a problem hiding this comment. [Style] Was the
Suggested change
Here and with There was a problem hiding this comment. Yes, because there's already a class called I have seen this in other projects, and it's fairly typical in languages like Java where you can define an interface separate from a class. Is there a definitive TypeScript way to handle this? "This" being, there's an interface I want a class to adhere to, and it's only used for that class. What should the name be? There was a problem hiding this comment. Quick answer: there isn't a single standard, but a name like Longer answer: the "interface only used for one class" pattern isn't common in TypeScript-land. If the interface is really only used for that one class then most of the time folks would just use the class name. Since we're not beholden to classic Java-style class hierarchies, it's less common AFAIU to put everything in a single shape the way the Quickly sketching a theoretical vague equivalent: declare function createLanguage<Settings extends LanguageSettings>
(settings: Settings): Language<Settings>;
export const jsonLanguage = createLanguage({
fileType: "text",
lineStart: 1,
// ...
});In that world, there wouldn't be a need for a Not suggesting changes, just posting context for why this naming problem isn't as commonly solved. There was a problem hiding this comment. Hmm okay, thanks for the background and details. I think I'm going to stick with what I have. I understand it's not TypeScript convention, but there aren't a lot of good options for the way we're doing things with JS + TS type definitions that make what I'm doing clear. So, we can live with a bit of ugliness. |
||||||
| LangOptions: JSONLanguageOptions; | ||||||
| Code: IJSONSourceCode; | ||||||
| RootNode: DocumentNode; | ||||||
| Node: AnyNode; | ||||||
| }>; | ||||||
|
|
||||||
| export type JSONRuleDefinition< | ||||||
| JSONRuleOptions extends unknown[], | ||||||
| JSONRuleMessageIds extends string = "", | ||||||
| > = RuleDefinition<{ | ||||||
| LangOptions: JSONLanguageOptions; | ||||||
| Code: IJSONSourceCode; | ||||||
| RuleOptions: JSONRuleOptions; | ||||||
| Visitor: JSONRuleVisitor; | ||||||
| Node: AnyNode; | ||||||
| MessageIds: JSONRuleMessageIds; | ||||||
| ExtRuleDocs: {}; | ||||||
| }>; | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| /** | ||
| * @fileoverview Rewrites import expressions for CommonJS compatibility. | ||
| * This script creates "dist/cjs/index.d.cts" from "dist/esm/index.d.ts" by modifying imports | ||
| * from `"./types.ts"` to `"./types.cts"`. | ||
| * | ||
| * @author Francesco Trotta | ||
| */ | ||
|
|
||
| import { readFile, writeFile } from "node:fs/promises"; | ||
|
|
||
| const oldSourceText = await readFile("dist/esm/index.d.ts", "utf-8"); | ||
| const newSourceText = oldSourceText.replaceAll( | ||
| 'import("./types.ts")', | ||
| 'import("./types.cts")', | ||
| ); | ||
| await writeFile("dist/cjs/index.d.cts", newSourceText); |
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.
I now see
dist/(cjs|esm)/types.d.tsanddst/(cjs|esm)/types.tsfiles locally. They're identical other than comments and an auto-generatedexport {}. I don't see anything importing from thetypes.d.ts. Is the duplication intentional?FWIW I believe it's more common to have just
.d.tsfiles. My instinct is that the expected path here would be to just havedist/(cjs|esm)/types.d.tsand typedefs use{import("./types.d.ts")}.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.
Because we run
tscondist/esmanddist/cjs,types.tsneeds to be present in both of those directories in order for the project to compile.types.d.tsis output bytscfromtypes.tsThere 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.
Gotcha. I tried renaming
src/types.tstosrc/types.d.tswith a find-and-replace. I think it's all working here: JoshuaKGoldberg@631810d. At leastnpm run buildpasses and the imports all work in editor.