-
-
Notifications
You must be signed in to change notification settings - Fork 5
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: Use updated types from @eslint/core #66
base: main
Are you sure you want to change the base?
Conversation
It looks like the type tests are failing because the json/tests/types/tsconfig.json Line 6 in a6c0bc9
We could remove that option for the moment since it's only used in the tests. In the long term I think it would be good to make sure that the |
Can you explain what the compatibility issue is? And why does running |
The issue is in the definition of export interface RuleVisitor {
/**
* Called for each node in the AST or at specific times during the traversal.
*/
[key: string]: (...args: any[]) => void;
} When the TypeScript For example: let ruleVisitor: RuleVisitor = {};
ruleVisitor.foo = undefined; // Error with strictNullChecks, else OK
interface Bar extends RuleVisitor {
bar?(): void; // Error with strictNullChecks, else OK
} The Lines 58 to 84 in fdde59d
It's possible that there are more incompatibilities in the core types but in this is the only one that's causing troubles with this PR. I tried manually changing the export type RuleVisitor = {
/**
* Called for each node in the AST or at specific times during the traversal.
*/
- [key: string]: (...args: any[]) => void;
+ [key in string]?: (...args: any[]) => void;
};
Because the |
Ah! That is very helpful, thank you. I think we should change |
Okay, looks like the |
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.
When I build the package everything works well but if I try to import the built package into another TypeScript package with "type": "commonjs"
, I'm getting errors:
node_modules/@eslint/json/dist/cjs/index.d.cts(12,38): error TS1542: Type import of an ECMAScript module from a CommonJS module must have a 'resolution-mode' attribute.
node_modules/@eslint/json/dist/cjs/index.d.cts(13,40): error TS1542: Type import of an ECMAScript module from a CommonJS module must have a 'resolution-mode' attribute.
node_modules/@eslint/json/dist/cjs/index.d.cts(17,36): error TS1542: Type import of an ECMAScript module from a CommonJS module must have a 'resolution-mode' attribute.
node_modules/@eslint/json/dist/cjs/index.d.cts(18,42): error TS1542: Type import of an ECMAScript module from a CommonJS module must have a 'resolution-mode' attribute.
node_modules/@eslint/json/dist/cjs/index.d.cts(20,52): error TS1542: Type import of an ECMAScript module from a CommonJS module must have a 'resolution-mode' attribute.
node_modules/@eslint/json/dist/cjs/index.d.cts(23,48): error TS1542: Type import of an ECMAScript module from a CommonJS module must have a 'resolution-mode' attribute.
node_modules/@eslint/json/dist/cjs/index.d.cts(25,51): error TS1542: Type import of an ECMAScript module from a CommonJS module must have a 'resolution-mode' attribute.
node_modules/@eslint/json/node_modules/@eslint/plugin-kit/dist/cjs/index.d.cts(12,35): error TS1542: Type import of an ECMAScript module from a CommonJS module must have a 'resolution-mode' attribute.
node_modules/@eslint/json/node_modules/@eslint/plugin-kit/dist/cjs/index.d.cts(13,36): error TS1542: Type import of an ECMAScript module from a CommonJS module must have a 'resolution-mode' attribute.
I think this is because types.ts
is considered an ESM module due to the .ts
extension, but it's re-exported by dist/cts/index.d.cts
which is CommonJS. I'll try to set up a repro.
import type { | ||
RuleVisitor, | ||
TextSourceCode, | ||
Language, | ||
LanguageOptions, | ||
RuleDefinition, | ||
} from "@eslint/core"; |
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.
@eslint/core
and is imported in the types. Should it be a runtime dependency?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If references to @eslint/core
are in the distributed .d.ts
files, then I believe @eslint/core
will need to be some kind of a runtime dependency. Otherwise end-users might import types from this package and get type errors on @eslint/core
not being found.
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".
Co-authored-by: Francesco Trotta <[email protected]>
That's strange. This is set up the same way I have the packages in the |
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'm not confident enough in this repo to explicitly approve or request changes, but left some comments and questions 🙂
): string; | ||
} | ||
|
||
export type IJSONLanguage = Language<{ |
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.
[Style] Was the I
an intentional addition? It's not typical in TypeScript code.
export type IJSONLanguage = Language<{ | |
export type JSONLanguage = Language<{ |
Here and with IJSONSourceCode
.
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.
Yes, because there's already a class called JSONLanguage
, so the type is IJSONLanguage
. The I
stands for interface.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick answer: there isn't a single standard, but a name like JSONLanguageLike
would be reasonable.
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 @eslint/core
Language
type is set up. A lot of architectures avoid classes altogether and instead just go with generic factory functions.
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 JSONLanguage
to be explicitly declared. It could be inferred from typeof jsonLanguage
with a helper.
Not suggesting changes, just posting context for why this naming problem isn't as commonly solved.
copy({ | ||
targets: [ | ||
{ src: "src/types.ts", dest: "dist/cjs" }, | ||
{ src: "src/types.ts", dest: "dist/esm" }, |
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.ts
and dst/(cjs|esm)/types.ts
files locally. They're identical other than comments and an auto-generated export {}
. I don't see anything importing from the types.d.ts
. Is the duplication intentional?
FWIW I believe it's more common to have just .d.ts
files. My instinct is that the expected path here would be to just have dist/(cjs|esm)/types.d.ts
and 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 tsc
on dist/esm
and dist/cjs
, types.ts
needs to be present in both of those directories in order for the project to compile. types.d.ts
is output by tsc
from types.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.
Gotcha. I tried renaming src/types.ts
to src/types.d.ts
with a find-and-replace. I think it's all working here: JoshuaKGoldberg@631810d. At least npm run build
passes and the imports all work in editor.
import type { | ||
RuleVisitor, | ||
TextSourceCode, | ||
Language, | ||
LanguageOptions, | ||
RuleDefinition, | ||
} from "@eslint/core"; |
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.
If references to @eslint/core
are in the distributed .d.ts
files, then I believe @eslint/core
will need to be some kind of a runtime dependency. Otherwise end-users might import types from this package and get type errors on @eslint/core
not being found.
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".
/** | ||
* The `SourceCode` implementation for JSON files. | ||
*/ | ||
export interface IJSONSourceCode |
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.
[Style] Was the I
an intentional addition? It's not typical in TypeScript code.
export interface IJSONSourceCode | |
export interface JSONSourceCode |
Here and with IJSONLanguage
.
Aside: I've always found this discussion around the choice to not preserve that naming in TypeScript ... interesting: microsoft/TypeScript-Handbook#121
Co-authored-by: Josh Goldberg ✨ <[email protected]>
Prerequisites checklist
What is the purpose of this pull request?
Update the plugin to use the latest types from
@eslint/core
.What changes did you make? (Give an overview)
types.ts
files to define types.RuleDefinition
typeJSONLanguage
to useIJSONLanguage
typeJSONSourceCode
to useIJSONSourceCode
typeRelated Issues
Is there anything you'd like reviewers to focus on?
The types test fails with this, and I don't understand why. The project builds just fine. Maybe @fasttime can take a look?