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

docs: added JS doc comments and tsconfig #530

Closed
wants to merge 4 commits into from

Conversation

srijan-paul
Copy link

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
Copy link

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
Copy link

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

1 similar comment
@eslint-github-bot
Copy link

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

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

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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

espree.js Outdated
* @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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this into a typedef too?

lib/options.js Outdated
* @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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this into a typedef?

lib/espree.js Outdated
* @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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@srijan-paul
Copy link
Author

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

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good point

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

lib/espree.js Outdated Show resolved Hide resolved
@@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

  • 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
Copy link
Member

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 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
Copy link
Contributor

@brettz9 brettz9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

package.json Outdated
@@ -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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

brettz9 commented Feb 2, 2022

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?

@nzakas nzakas linked an issue Feb 19, 2022 that may be closed by this pull request
@srijan-paul
Copy link
Author

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

@srijan-paul srijan-paul force-pushed the espree-ts-decls branch 2 times, most recently from 4b4b41b to 24d1358 Compare March 12, 2022 15:12
Co-authored-by: Milos Djermanovic <[email protected]>
Co-authored-by: Bryan Mishkin <[email protected]>
Co-authored-by: Milos Djermanovic <[email protected]>
Co-authored-by: Bryan Mishkin <[email protected]>
@srijan-paul
Copy link
Author

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
Copy link
Author

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
Copy link
Member

nzakas commented Apr 12, 2022

@brettz9 @mdjermanovic any followups here?

Copy link
Contributor

@brettz9 brettz9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

mdjermanovic commented Apr 14, 2022

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).

espree.js Outdated
* @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.
Copy link
Member

@mdjermanovic mdjermanovic Apr 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

https://github.com/eslint/espree/blob/b578a66991985d96d5e6ee4f388c4356ad0b3594/lib/espree.js#L154-L161

@mdjermanovic
Copy link
Member

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

"shelljs": "^0.3.0"
"shelljs": "^0.3.0",
"typescript": "^4.5.4",
"unicode-6.3.0": "^0.7.5"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're not using this dependency.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

espree.js Outdated
@@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 36 to 37
* @property {number|undefined} start start column.
* @property {number|undefined} end end column.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/**
* @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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

lib/espree.js Outdated
Comment on lines 18 to 19
* @property {number|undefined} start Start column of a comment.
* @property {number|undefined} end End column of the comment.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lib/espree.js Outdated
Comment on lines 21 to 22
* @property {acorn.Position} startLoc Start location of the comment.
* @property {acorn.Position} endLoc End location of the comment.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mdjermanovic
Copy link
Member

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

@nzakas
Copy link
Member

nzakas commented Apr 16, 2022

Token and Comment are probably fine

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 6, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@srijan-paul
Copy link
Author

srijan-paul commented May 6, 2022

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
Copy link
Member

btmills commented May 8, 2022

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!

@btmills btmills closed this May 8, 2022
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.

@types for espree?
7 participants