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

feat: Bundle JSDoc-built TypeScript declaration file #34

Merged
merged 16 commits into from Feb 11, 2022

Conversation

brettz9
Copy link
Contributor

@brettz9 brettz9 commented Feb 6, 2022

In the process of preparing ESLint for more type awareness, I figured we might avoid the need for an external @types package with the ability to maintain the types here (and to generate it out of JSDoc where possible as per eslint/espree#529 (comment) ).

One item I noticed in comparing with the declaration file in @types/eslint-visitor-keys is that undefined was allowed on properties, e.g.:

export interface VisitorKeys {
    readonly [type: string]: ReadonlyArray<string> | undefined;
}

In the PR, I have sought to preserve this ability, but I noticed that with the unionWith function, the additional keys argument couldn't include such properties as the current code would fail.

@eslint-github-bot

This comment was marked as resolved.

@eslint-github-bot

This comment was marked as resolved.

@brettz9 brettz9 changed the title Bundle declaration file feat: bundle JSDoc-built TypeScript declaration file Feb 6, 2022
@brettz9 brettz9 changed the title feat: bundle JSDoc-built TypeScript declaration file feat: Bundle JSDoc-built TypeScript declaration file Feb 6, 2022
@brettz9

This comment was marked as resolved.

@brettz9

This comment was marked as resolved.

.eslintrc.json Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
lib/visitor-keys.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@mdjermanovic
Copy link
Member

index.d.ts currently looks like this:

/**
 * Get visitor keys of a given node.
 * @param {object} node The AST node to get keys.
 * @returns {readonly string[]} Visitor keys of the node.
 */
export function getKeys(node: object): readonly string[];
/**
 * Make the union set with `KEYS` and given keys.
 * @param {KeysStrictReadonly} additionalKeys The additional keys.
 * @returns {KeysStrictReadonly} The union set.
 */
export function unionWith(additionalKeys: KeysStrictReadonly): KeysStrictReadonly;
export { KEYS };
export type KeysStrict = {
    [type: string]: readonly string[];
};
export type KeysStrictReadonly = {
    readonly [type: string]: readonly string[];
};
import KEYS from "./visitor-keys.js";
//# sourceMappingURL=index.d.ts.map

It exports some types, so someone might use them in their project. Should we aim for this to be stable (e.g., to avoid renaming exported types), and would it be possible to add some tests for index.d.ts?

@brettz9
Copy link
Contributor Author

brettz9 commented Feb 9, 2022

... would it be possible to add some tests for index.d.ts?

FWIW, @types/eslint-visitor-keys has its declaration (from https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/eslint-visitor-keys/index.d.ts )

export interface VisitorKeys {
    readonly [type: string]: ReadonlyArray<string> | undefined;
}

export const KEYS: VisitorKeys;
export function getKeys(node: {}): ReadonlyArray<string>;
export function unionWith(keys: VisitorKeys): VisitorKeys;

...and has some kind of pseudo-tests at https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/eslint-visitor-keys/eslint-visitor-keys-tests.ts .

TypeScript itself recommends tsd. For coverage though, tsd does not currently do so (there are some TypeScript test libraries which perform coverage, but I'm not sure that the libraries that do so would cover declaration files anyways--perhaps maybe only checking executable portions).

lib/index.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@brettz9
Copy link
Contributor Author

brettz9 commented Feb 10, 2022

I looked into preparing tests, but ran into an issue with the tsd testing framework, so unless you want plain TypeScript as the "tests", I don't know if you want any such testing in a separate PR.

@mdjermanovic
Copy link
Member

I looked into preparing tests, but ran into an issue with the tsd testing framework, so unless you want plain TypeScript as the "tests", I don't know if you want any such testing in a separate PR.

I think it isn't critical to test readonly. If we could make tests that confirm that correct use of the APIs of this library does not cause typescript errors, that seems good enough for the start.

@brettz9
Copy link
Contributor Author

brettz9 commented Feb 10, 2022

I looked into preparing tests, but ran into an issue with the tsd testing framework, so unless you want plain TypeScript as the "tests", I don't know if you want any such testing in a separate PR.

I think it isn't critical to test readonly. If we could make tests that confirm that correct use of the APIs of this library does not cause typescript errors, that seems good enough for the start.

Ok, I've added some simple tests.

I also added a commit on top of that to rename per your type renaming suggestions.

lib/index.js Outdated Show resolved Hide resolved
brettz9 and others added 2 commits February 11, 2022 09:01
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!

@mdjermanovic
Copy link
Member

It looks like all comments by @nzakas have been addressed, so I'm merging this now for the release.

The two new files we'll publish now have the following content.

dist/index.d.ts:

/**
 * Get visitor keys of a given node.
 * @param {object} node The AST node to get keys.
 * @returns {readonly string[]} Visitor keys of the node.
 */
export function getKeys(node: object): readonly string[];
/**
 * Make the union set with `KEYS` and given keys.
 * @param {VisitorKeys} additionalKeys The additional keys.
 * @returns {VisitorKeys} The union set.
 */
export function unionWith(additionalKeys: VisitorKeys): VisitorKeys;
export { KEYS };
export type VisitorKeys = {
    readonly [type: string]: readonly string[];
};
import KEYS from "./visitor-keys.js";
//# sourceMappingURL=index.d.ts.map

dist/visitor-keys.d.ts:

export default KEYS;
export type VisitorKeys = import('./index.js').VisitorKeys;
/**
 * @typedef {import('./index.js').VisitorKeys} VisitorKeys
 */
/**
 * @type {VisitorKeys}
 */
declare const KEYS: VisitorKeys;
//# sourceMappingURL=visitor-keys.d.ts.map

I verified that DefinitelyTyped/types/eslint-visitor-keys/eslint-visitor-keys-tests.ts works well with eslint-visitor-keys package installed from this branch, so it's unlikely that this change will break anyone's tsc build. I've also verified that these declarations prevent invalid use of eslint-visitor-keys APIs in a TS code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants