docs: added JS doc comments and tsconfig #530

srijan-paul · 2022-01-24T16:32:54Z

Added JS doc comments and a TSConfig that should generate the type definitions.
Note that it still may require changes.
Things that I'm curious about:

Should we be testing these type definitions?
Are the AST returned by espree and acorn similar enough that using acorn.Node as the return type for espree.parse is safe?

eslint-github-bot · 2022-01-24T16:32:58Z

Hi @srijan-paul!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
There should be a space following the initial tag and colon, for example 'feat: Message'.
The first letter of the tag should be in lowercase

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

eslint-github-bot · 2022-01-24T16:37:31Z

Hi @srijan-paul!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
There should be a space following the initial tag and colon, for example 'feat: Message'.
The first letter of the tag should be in lowercase

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

eslint-github-bot · 2022-01-24T16:39:53Z

Hi @srijan-paul!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
There should be a space following the initial tag and colon, for example 'feat: Message'.
The first letter of the tag should be in lowercase

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

nzakas

This is some fantastic work, thank you! Just a couple small suggestions.

In order for Espree to ship the type definitions, we also need to make these changes to package.json:

Add dist/espree.d.ts to the files array.
Add a types property that points to dist/espree.d.ts

nzakas · 2022-01-25T00:34:40Z

espree.js

- * @param {Object} options Options defining how to tokenize.
- * @returns {Token[]} An array of tokens.
+ * @param {ParserOptions} options Options defining how to tokenize.
+ * @returns {import("./lib/token-translator.js").EsprimaToken[]} An array of tokens.


Can we move this into a typedef too?

nzakas · 2022-01-25T00:41:22Z

lib/options.js

+ * @property {number|"latest"} ecmaVersion The ECMAScript version to use (between 3 and 13, or 2015 and 2022, or "latest").
+ * @property {boolean} allowReserved Only allowed when `ecmaVersion` is set to 3.
+ * @property {"script"|"module"|"commonjs"} sourceType The kind of the source string being parsed.
+ * @property {import("./features").EcmaFeatures} ecmaFeatures The additional features to enable.


Can we move this into a typedef?

nzakas · 2022-01-25T00:42:35Z

lib/espree.js

+ * @property {string} text Contents of the comment.
+ * @property {number|undefined} start Start column of a comment.
+ * @property {number|undefined} end End column of the comment.
+ * @property {[number, number]|undefined} range The [start, end] range of a comment.


Since [number,number] is also used in EsprimaToken, can we move this into its own typedef for clarity?

srijan-paul · 2022-01-25T13:05:54Z

Whoops, I missed adding the build file to package.json. Fixed!

nzakas

Looking good! One last step: we need to ensure that tsc is run. In package.json, can you add tsc to the end of the build script?

bmish · 2022-01-27T18:04:26Z

tsconfig.json

@@ -0,0 +1,10 @@
+{
+  "include": ["lib/**/*", "espree.js"],
+  "compilerOptions": {


Can we use "strict": true in here? https://www.typescriptlang.org/tsconfig#strict

Ah, good point

I'm not sure it has any effects - we don't have ts code, and didn't enable "checkJs": true.

Hmm, you could be right, I thought there might be some stricter behavior/validation it might enable. Regardless, it probably wouldn't hurt to use it, especially in case we add "checkJs": true later.

tsconfig.json

lib/espree.js

bmish · 2022-01-29T16:19:15Z

lib/espree.js

@@ -232,7 +252,7 @@ export default () => Parser => {

        /**
         * Overwrites the default raise method to throw Esprima-style errors.
-         * @param {int} pos The position of the error.
+         * @param {number} pos The position of the error.


Just wondering, this isn't related to acorn.Position is it?

No, this was presumably done to simplify the fact that TS (and JS) doesn't distinguish regular integers and the number type. I think it would be ideal to preserve this though so as to clarify the intent, e.g., for autocomplete.

But then passing a number wouldn't satisfy TS's typechecker, no?
I'd rather have it be integer too but I think we can mention that in the parameter description since TS/JS don't have native integers.

int would need to have its own typedef set to number (but if it's a problem because TypeScript will export the type, don't worry about it).

mdjermanovic · 2022-01-31T13:59:33Z

Should we be testing these type definitions?

I think that would be good. Otherwise, we won't know if the definitions are broken, which would make Espree unusable in typescript projects?

Speaking of that, npm run build produces the following espree.d.ts:

/**
 * Tokenizes the given code.
 * @param {string} code The code to tokenize.
 * @param {ParserOptions} options Options defining how to tokenize.
 * @returns {EsprimaToken[]} An array of tokens.
 * @throws {SyntaxError} If the input code is invalid.
 * @private
 */
export function tokenize(code: string, options: ParserOptions): EsprimaToken[];
/**
 * Parses the given code.
 * @param {string} code The code to tokenize.
 * @param {ParserOptions} options Options defining how to tokenize.
 * @returns {acorn.Node} The "Program" AST node.
 * @throws {SyntaxError} If the input code is invalid.
 */
export function parse(code: string, options: ParserOptions): acorn.Node;
export const version: "main";
export const Syntax: {};
export const VisitorKeys: any;
export const latestEcmaVersion: number;
export const supportedEcmaVersions: number[];
export type acorn = typeof acorn;
export type ParserOptions = import("./lib/options").ParserOptions;
export type EsprimaToken = import("./lib/token-translator").EsprimaToken;
export type TokenRange = import("./lib/token-translator").TokenRange;
import * as acorn from "acorn";
//# sourceMappingURL=espree.d.ts.map

When I open espree.d.ts in VSCode, it reports one error:

Namespace '"d:/tmp/espree-types/espree/dist/lib/token-translator"' has no exported member 'TokenRange'.

Also, when I try to use this version from another project (I added "types": "dist/espree.d.ts" in node_modules/espree/package.json, and copied all published files including dist/espree.d.ts), those relative paths seems to be a problem for tsc:

//----------- test.ts ---------------
import * as espree from "espree";

> tsc

node_modules/espree/dist/espree.d.ts:24:36 - error TS2307: Cannot find module './lib/options' or its corresponding type declarations.

24 export type ParserOptions = import("./lib/options").ParserOptions;
                                      ~~~~~~~~~~~~~~~

node_modules/espree/dist/espree.d.ts:25:35 - error TS2307: Cannot find module './lib/token-translator' or its corresponding type declarations.

25 export type EsprimaToken = import("./lib/token-translator").EsprimaToken;
                                     ~~~~~~~~~~~~~~~~~~~~~~~~

node_modules/espree/dist/espree.d.ts:26:33 - error TS2307: Cannot find module './lib/token-translator' or its corresponding type declarations.

26 export type TokenRange = import("./lib/token-translator").TokenRange;
                                   ~~~~~~~~~~~~~~~~~~~~~~~~


Found 3 errors.

mdjermanovic · 2022-01-31T15:13:49Z

Also, when I try to use this version from another project (I added "types": "dist/espree.d.ts" in node_modules/espree/package.json, and copied all published files including dist/espree.d.ts), those relative paths seems to be a problem for tsc:

//----------- test.ts ---------------
import * as espree from "espree";

> tsc

node_modules/espree/dist/espree.d.ts:24:36 - error TS2307: Cannot find module './lib/options' or its corresponding type declarations.

24 export type ParserOptions = import("./lib/options").ParserOptions;
                                      ~~~~~~~~~~~~~~~

node_modules/espree/dist/espree.d.ts:25:35 - error TS2307: Cannot find module './lib/token-translator' or its corresponding type declarations.

25 export type EsprimaToken = import("./lib/token-translator").EsprimaToken;
                                     ~~~~~~~~~~~~~~~~~~~~~~~~

node_modules/espree/dist/espree.d.ts:26:33 - error TS2307: Cannot find module './lib/token-translator' or its corresponding type declarations.

26 export type TokenRange = import("./lib/token-translator").TokenRange;
                                   ~~~~~~~~~~~~~~~~~~~~~~~~


Found 3 errors.

tsc actually creates dist/espree.d.ts and dist/lib/ with a .d.ts file for each .js file from lib/:

.
└── dist/
    ├── espree.cjs 
    ├── espree.cjs.map
    ├── espree.d.ts
    ├── espree.d.ts.map
    └── lib/
        ├── ast-node-types.d.ts
        ├── ast-node-types.d.ts.map
        ├── espree.d.ts
        ├── espree.d.ts.map
        ├── features.d.ts
        ├── features.d.ts.map
        ├── options.d.ts
        ├── options.d.ts.map
        ├── token-translator.d.ts
        ├── token-translator.d.ts.map
        ├── version.d.ts
        └── version.d.ts.map

Should we then also add "dist/lib" to "files" in package.json?

espree.js

lib/options.js

lib/features.js

brettz9

FWIW, I had neglected to see this and started my own PR which does do checkJs, though it is not finished and probably would need changes to Acorn since Acorn declaration files apparently currently underspecify the parser, and I could rebase anyways. In any case, this was helpful to have.

I'd suggest keeping the int type for clarifying intent and I think the @types/acorn can be dropped given Acorn's own more current support. But otherwise LGTM...

brettz9 · 2022-02-02T03:32:22Z

package.json

@@ -39,6 +41,7 @@
    "@rollup/plugin-commonjs": "^17.1.0",
    "@rollup/plugin-json": "^4.1.0",
    "@rollup/plugin-node-resolve": "^11.2.0",
+    "@types/acorn": "^4.0.6",


Acorn itself has a *.d.ts file and the mod date appears more recent, so is this this still necessary?

brettz9 · 2022-02-02T03:35:02Z

lib/espree.js

@@ -232,7 +252,7 @@ export default () => Parser => {

        /**
         * Overwrites the default raise method to throw Esprima-style errors.
-         * @param {int} pos The position of the error.
+         * @param {number} pos The position of the error.


No, this was presumably done to simplify the fact that TS (and JS) doesn't distinguish regular integers and the number type. I think it would be ideal to preserve this though so as to clarify the intent, e.g., for autocomplete.

brettz9 · 2022-02-02T03:50:01Z

Also, when I try to use this version from another project (I added "types": "dist/espree.d.ts" in node_modules/espree/package.json, and copied all published files including dist/espree.d.ts), those relative paths seems to be a problem for tsc:

//----------- test.ts ---------------
import * as espree from "espree";

> tsc

node_modules/espree/dist/espree.d.ts:24:36 - error TS2307: Cannot find module './lib/options' or its corresponding type declarations.

24 export type ParserOptions = import("./lib/options").ParserOptions;
                                      ~~~~~~~~~~~~~~~

node_modules/espree/dist/espree.d.ts:25:35 - error TS2307: Cannot find module './lib/token-translator' or its corresponding type declarations.

25 export type EsprimaToken = import("./lib/token-translator").EsprimaToken;
                                     ~~~~~~~~~~~~~~~~~~~~~~~~

node_modules/espree/dist/espree.d.ts:26:33 - error TS2307: Cannot find module './lib/token-translator' or its corresponding type declarations.

26 export type TokenRange = import("./lib/token-translator").TokenRange;
                                   ~~~~~~~~~~~~~~~~~~~~~~~~


Found 3 errors.

tsc actually creates dist/espree.d.ts and dist/lib/ with a .d.ts file for each .js file from lib/:

.
└── dist/
    ├── espree.cjs 
    ├── espree.cjs.map
    ├── espree.d.ts
    ├── espree.d.ts.map
    └── lib/
        ├── ast-node-types.d.ts
        ├── ast-node-types.d.ts.map
        ├── espree.d.ts
        ├── espree.d.ts.map
        ├── features.d.ts
        ├── features.d.ts.map
        ├── options.d.ts
        ├── options.d.ts.map
        ├── token-translator.d.ts
        ├── token-translator.d.ts.map
        ├── version.d.ts
        └── version.d.ts.map

Should we then also add "dist/lib" to "files" in package.json?

TypeScript allows an outFile option. Maybe that would prevent the need?

srijan-paul · 2022-03-11T19:36:56Z

Will update this over the weekend. Got caught with life there.

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com> Co-authored-by: Bryan Mishkin <698306+bmish@users.noreply.github.com>

srijan-paul · 2022-03-12T17:45:49Z

Updated the PR (squashed some commits). Should be ready to go now :)

@mdjermanovic I've fixed the issue with incorrectly generated .d.ts. I'm not sure what the best way would be to test the d.ts files though. Suggestions welcome.
@brettz9 Thanks for pointing out the issues with comments. Fixed.

srijan-paul · 2022-04-06T11:37:40Z

Hya, not to rush anyone, but I was wondering if I could get some feedback on this PR since I'd be needing the type definitions in a typescript project I'm working on :p

nzakas · 2022-04-12T00:55:55Z

@brettz9 @mdjermanovic any followups here?

brettz9

My suggestion to add int to its own typedef (e.g., /** @typedef {number} int */) wasn't addressed, but it is not important.

Am preoccupied with other work to do a more careful review of the subsequent changes, but it had looked good to me previously.

mdjermanovic · 2022-04-14T11:34:55Z

I'm still seeing the problems described in #530 (comment):

import("./lib/token-translator").TokenRange - TokenRange type doesn't exist in that module.
The published declaration file imports declaration files that are not published (you can run npm pack to generate a .tgz file, and then install Espree from that .tgz file in a TypeScript project to test).

mdjermanovic · 2022-04-14T12:48:16Z

espree.js

- * @param {Object} options Options defining how to tokenize.
- * @returns {ASTNode} The "Program" AST node.
+ * @param {ParserOptions} options Options defining how to tokenize.
+ * @returns {acorn.Node} The "Program" AST node.


Object returned from parse() also has sourceType property, and (optionally) comments and tokens properties.

espree/lib/espree.js

Lines 154 to 161 in b578a66

program.sourceType = extra.originalSourceType;

if (extra.comments) {

program.comments = extra.comments;

}

if (extra.tokens) {

program.tokens = extra.tokens;

}

mdjermanovic · 2022-04-14T12:56:12Z

I'm not sure what the best way would be to test the d.ts files though. Suggestions welcome.

Maybe we could use tsd, like in https://github.com/eslint/eslint-visitor-keys

mdjermanovic · 2022-04-14T14:01:20Z

package.json

-    "shelljs": "^0.3.0"
+    "shelljs": "^0.3.0",
+    "typescript": "^4.5.4",
+    "unicode-6.3.0": "^0.7.5"


I think we're not using this dependency.

We're using it in tests/lib/libraries.js

mdjermanovic · 2022-04-14T14:01:50Z

package.json

@@ -69,7 +73,7 @@
    "test": "npm-run-all -p unit lint",
    "lint": "eslint \"*.?(c)js\" lib/ tests/lib/",
    "fixlint": "npm run lint -- --fix",
-    "build": "rollup -c rollup.config.js",
+    "build": "rollup -c rollup.config.js && tsc --build",


Why do we need the --build flag, what is the difference?

mdjermanovic · 2022-04-14T14:28:28Z

espree.js

@@ -124,8 +131,8 @@ export function tokenize(code, options) {
 /**
 * Parses the given code.
 * @param {string} code The code to tokenize.
- * @param {Object} options Options defining how to tokenize.
- * @returns {ASTNode} The "Program" AST node.
+ * @param {ParserOptions} options Options defining how to tokenize.


Suggested change

* @param {ParserOptions} options Options defining how to tokenize.

* @param {ParserOptions} [options] Options defining how to parse.

mdjermanovic · 2022-04-14T14:53:05Z

lib/token-translator.js

+ * @property {number|undefined} start start column.
+ * @property {number|undefined} end end column.


These are not columns, but indexes (like range[0] and range[1])

mdjermanovic · 2022-04-14T15:06:24Z

lib/espree.js

+/**
+ * @typedef {Object} EsprimaComment
+ * @property {"Block"|"Line"} type Type of the comment, can either be "Block" (multiline) or "Line" (single line).
+ * @property {string} text Contents of the comment.


Suggested change

* @property {string} text Contents of the comment.

* @property {string} value Contents of the comment.

mdjermanovic · 2022-04-14T15:06:52Z

lib/espree.js

+ * @property {number|undefined} start Start column of a comment.
+ * @property {number|undefined} end End column of the comment.


Same note as https://github.com/eslint/espree/pull/530/files#r850527828

mdjermanovic · 2022-04-14T15:10:29Z

lib/espree.js

+ * @property {acorn.Position} startLoc Start location of the comment.
+ * @property {acorn.Position} endLoc End location of the comment.


There are no startLoc and endLoc properties on comments. They have loc, like tokens.

mdjermanovic · 2022-04-14T16:30:13Z

Do we want to export types named EsprimaToken and EsprimaComment, or would it be better to name them just Token and Comment?

nzakas · 2022-04-16T00:56:06Z

Token and Comment are probably fine

linux-foundation-easycla · 2022-05-06T04:31:16Z

The committers listed above are authorized under a signed CLA.

✅ login: srijan-paul / name: injuly (f8ce0d2, 41d48d8, 625af0c)

srijan-paul · 2022-05-06T04:50:44Z

Ack. Looking for a way to fix the issue with broken imports in the published package.
EDIT: adding dist/lib to files in package.json fixed the issue.

btmills · 2022-05-08T00:34:18Z

Hi @srijan-paul, thank you for your work on this! Since this and #544 are attacking the same problem but #544 additionally enables type checking in the repository, we'd like to focus our efforts on #544. Please know that we appreciate your work on this so far, and if you haven't already heard from him, nzakas will be reaching out because this is exactly the sort of improvement that our contributor pool payments were designed for. I'm sorry this change won't be getting merged, but I hope to see what we've learned from this work reflected in #544 and would welcome seeing your feedback there!

eslint-github-bot bot added the triage label Jan 24, 2022

srijan-paul force-pushed the espree-ts-decls branch from 695e582 to c86b367 Compare January 24, 2022 16:37

srijan-paul changed the title ~~Espree TS decls~~ docs: added JS doc comments and tsconfig Jan 24, 2022

eslint-github-bot bot added the documentation label Jan 24, 2022

srijan-paul mentioned this pull request Jan 24, 2022

@types for espree? #529

Open

nzakas requested changes Jan 25, 2022

View reviewed changes

srijan-paul requested a review from nzakas January 25, 2022 13:05

nzakas mentioned this pull request Jan 27, 2022

Roadmap planning 2022 eslint/eslint#15383

Closed

nzakas requested changes Jan 27, 2022

View reviewed changes

bmish reviewed Jan 27, 2022

View reviewed changes

aladdin-add reviewed Jan 29, 2022

View reviewed changes

tsconfig.json Show resolved Hide resolved

srijan-paul requested review from nzakas and bmish January 29, 2022 15:56

bmish approved these changes Jan 29, 2022

View reviewed changes

mdjermanovic reviewed Jan 31, 2022

View reviewed changes

espree.js Outdated Show resolved Hide resolved

espree.js Outdated Show resolved Hide resolved

lib/options.js Outdated Show resolved Hide resolved

lib/features.js Outdated Show resolved Hide resolved

brettz9 suggested changes Feb 2, 2022

View reviewed changes

nzakas linked an issue Feb 19, 2022 that may be closed by this pull request

@types for espree? #529

Open

srijan-paul force-pushed the espree-ts-decls branch 2 times, most recently from 4b4b41b to 24d1358 Compare March 12, 2022 15:12

docs: add jsdoc types to espree

f8ce0d2

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com> Co-authored-by: Bryan Mishkin <698306+bmish@users.noreply.github.com>

srijan-paul force-pushed the espree-ts-decls branch from 24d1358 to f8ce0d2 Compare March 12, 2022 15:18

docs: add jsdoc types to espree

41d48d8

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com> Co-authored-by: Bryan Mishkin <698306+bmish@users.noreply.github.com>

srijan-paul requested review from bmish and brettz9 March 12, 2022 17:43

brettz9 approved these changes Apr 12, 2022

View reviewed changes

mdjermanovic reviewed Apr 14, 2022

View reviewed changes

nzakas added the tsc agenda label May 5, 2022

srijan-paul force-pushed the espree-ts-decls branch 4 times, most recently from 625af0c to cdc2891 Compare May 6, 2022 04:42

srijan-paul requested a review from mdjermanovic May 6, 2022 04:42

srijan-paul force-pushed the espree-ts-decls branch from cdc2891 to 6499862 Compare May 6, 2022 04:44

fix: fix some inconsistent type definitions

b7a1e85

srijan-paul force-pushed the espree-ts-decls branch from 6499862 to b7a1e85 Compare May 6, 2022 04:47

fix: add missing type definition files

63f5b65

btmills closed this May 8, 2022

btmills removed the tsc agenda label May 19, 2022

	program.sourceType = extra.originalSourceType;

	if (extra.comments) {
	program.comments = extra.comments;
	}
	if (extra.tokens) {
	program.tokens = extra.tokens;
	}

	* @param {ParserOptions} options Options defining how to tokenize.
	* @param {ParserOptions} [options] Options defining how to parse.

		* @property {number\|undefined} start start column.
		* @property {number\|undefined} end end column.

	* @property {string} text Contents of the comment.
	* @property {string} value Contents of the comment.

		* @property {acorn.Position} startLoc Start location of the comment.
		* @property {acorn.Position} endLoc End location of the comment.

docs: added JS doc comments and tsconfig #530

docs: added JS doc comments and tsconfig #530

Conversation

srijan-paul commented Jan 24, 2022

eslint-github-bot bot commented Jan 24, 2022

eslint-github-bot bot commented Jan 24, 2022

eslint-github-bot bot commented Jan 24, 2022

nzakas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srijan-paul commented Jan 25, 2022

nzakas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdjermanovic commented Jan 31, 2022

mdjermanovic commented Jan 31, 2022

brettz9 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brettz9 commented Feb 2, 2022

srijan-paul commented Mar 11, 2022

srijan-paul commented Mar 12, 2022

srijan-paul commented Apr 6, 2022

nzakas commented Apr 12, 2022

brettz9 left a comment

Choose a reason for hiding this comment

mdjermanovic commented Apr 14, 2022 • edited

mdjermanovic Apr 14, 2022 • edited

Choose a reason for hiding this comment

mdjermanovic commented Apr 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdjermanovic commented Apr 14, 2022

nzakas commented Apr 16, 2022

linux-foundation-easycla bot commented May 6, 2022 • edited

srijan-paul commented May 6, 2022 • edited

btmills commented May 8, 2022

brettz9 left a comment •

edited

mdjermanovic commented Apr 14, 2022 •

edited

mdjermanovic Apr 14, 2022 •

edited

linux-foundation-easycla bot commented May 6, 2022 •

edited

srijan-paul commented May 6, 2022 •

edited