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: Programmatic building of type-checkable JS and declaration files #544

Closed
wants to merge 24 commits into from

Conversation

brettz9
Copy link
Contributor

@brettz9 brettz9 commented Apr 20, 2022

Here's a preliminary approach toward fixing #529

Note that this PR also depends on changes to acorn (already merged but not yet released now released) and acorn-jsx (not yet reviewed). One has to run npm link ../acorn-jsx (assuming one has that project and using my PR for acorn-jsx to those projects)

I still need to see how well the built declaration files work practically speaking. I've confirmed the declaration file builds can work (checking through a project I npm linked to espree). One additional gotcha in testing this way is that one also has to ensure that acorn-jsx's own node_modules/acorn/dist/acorn.d.ts file is updated to have the same contents as acorn on master.

The build routine is I think pretty semantic (I didn't need the use of any tool to adjust ranges after all even with my preexisting approach, as the generator doesn't use ranges for much besides comments, and the JSDoc-based AST parser avoids the need to use comments).

The one hackish part is that because because I did not use a TypeScript transformer like ttypescript which might maintain better tracking of the type of variable in scope (e.g., toward building a necessary type cast for some super() call arguments), I had to hard-code a little handling. I could look into using such as ttypescript, but given a desire to get on with this, I think this solution might work if the declaration files indeed pan out as hoped. It should still be possible for users to make a number of changes to the skeleton of the parser without breaking the build process (though the tool is not generic enough to handle all manner of structures with the custom @export it supports, such as to build anything besides the class expression that we needed).

Submitting now both to telegraph that there is progress on this, and in case anyone with better TypeScript skills wanted to review the resulting declaration file for any concerns.

I've labeled it as a feature, as I expect a new declaration file is not merely about docs and could change some behavior, though perhaps not enough to warrant a breaking change (?)

I can simplify the PR somewhat by creating PRs for a few ancillary linting changes (e.g., to test files), but a lot of the refactoring you see is what it took to get checkJs working with TS, but it is still JS!

This PR also makes the following changes required by type-checking the JS:

  • feat(lib/espree): throws specific error if jsx_readString superclass undefined
  • refactor(lib/espree): changes to force EspreeParser constructor to convert string as created with new String to plain string (for typing)
  • refactor(lib/espree): checks for existence of firstNode.range and firstNode.loc in parse (as may be absent)
  • refactor(lib/espree): checks for existence of tokens in tokenize (as may be absent)
  • refactor(lib/espree): checks for existence of extra.lastToken.range and extra.lastToken.loc in parse (as may be absent)
  • refactor(lib/token-translator): checks for existence of lastTemplateToken.loc and lastTemplateToken.range (as may be absent)

@brettz9
Copy link
Contributor Author

brettz9 commented Apr 26, 2022

I've updated the description to reflect the fact that I did confirm this works as intended from outside (npm-link'ed) projects, and I did simplify the PR somewhat by rebasing on the unrelated changes I had originally included in this PR.

@brettz9 brettz9 force-pushed the tsc-acorn2-jsdoc-eslint-parser branch 2 times, most recently from 6450b48 to 9edf677 Compare April 26, 2022 07:18
espree.js Outdated
*
* `comment` is not in `acorn.Options` and doesn't err without it, but is used
* @typedef {{
* allowReserved?: boolean | "never",
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
* allowReserved?: boolean | "never",
* allowReserved?: boolean,

Only true and false are allowed values.

espree.js Outdated
Comment on lines 77 to 84
* ranges?: boolean,
* locations?: boolean,
* allowReturnOutsideFunction?: boolean,
* onToken?: ((token: acorn.Token) => any) | acorn.Token[],
* onComment?: ((
* isBlock: boolean, text: string, start: number, end: number, startLoc?: acorn.Position,
* endLoc?: acorn.Position
* ) => void) | acorn.Comment[],
Copy link
Member

Choose a reason for hiding this comment

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

ranges, locations, allowReturnOutsideFunction, onToken, and onComment are not valid options for Espree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks.

espree.js Outdated
* loc?: boolean,
* tokens?: boolean | null,
* comment?: boolean,
* } & jsx.Options} ParserOptions
Copy link
Member

Choose a reason for hiding this comment

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

acorn-jsx's options are not options for Espree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor Author

@brettz9 brettz9 May 5, 2022

Choose a reason for hiding this comment

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

@mdjermanovic : Upon digging back into why I did this, TypeScript can be insistent that no extra properties are added which are not specified. But since we do pass on these options to the underlying parser, including acorn-jsx, it seems to me that unless we know the available options should never be passed through, we do actually want them included (thankfully, acorn-jsx describes these properties as optional, so we could specify them by intersection here). (Otherwise, a user may be informed, they can't pass on those properties, even while acorn or acorn-jsx allows them.)

Likewise with the other properties I had set in the prior comments.

Unless you were indicating that this should simply not be allowed...

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 the only options passed to underlying parsers:

https://github.com/eslint/espree/blob/6c718af090c1b5dd25d74a2ecfc65fbee0c00716/lib/espree.js#L69-L79

(note that options object here is the return value of normalizeOptions, so it isn't the same as Espree options).

We intentionally don't allow options that are not Espree options to pass through Espree to underlying parsers. We don't check and throw errors if there are unknown options, but they are basically ignored.

Copy link
Member

Choose a reason for hiding this comment

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

Also, per the documentation it seems that acorn-jsx expects those two options in the jsx() call, not in the constructor. Since we are not passing anything to jsx(), they will be ignored if passed into Espree as Espree options.

(Otherwise, a user may be informed, they can't pass on those properties, even while acorn or acorn-jsx allows them.)

That's the point, user indeed can't pass on those properties because they would be just ignored by Espree and not passed through to acorn or acorn-jsx.

brettz9 and others added 3 commits April 28, 2022 18:44
Also:
- feat(`lib/espree`): throws specific error if `jsx_readString` superclass undefined
- refactor(`lib/espree`): changes to force EspreeParser constructor to convert string as created with `new String` to plain string (for typing)
- refactor(`lib/espree`): checks for existence of `firstNode.range` and `firstNode.loc` in `parse` (as may be absent)
- refactor(`lib/espree`): checks for existence of `tokens` in `tokenize` (as may be absent)
- refactor(`lib/espree`): checks for existence of `extra.lastToken.range` and `extra.lastToken.loc` in `parse` (as may be absent)
- refactor(`lib/token-translator``): checks for existence of `lastTemplateToken.loc` and `lastTemplateToken.range` (as may be absent)
- chore: update devDeps.
Co-authored-by: Milos Djermanovic <[email protected]>
@brettz9 brettz9 force-pushed the tsc-acorn2-jsdoc-eslint-parser branch from 6982298 to 403c952 Compare April 28, 2022 10:44
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.

Thanks for doing this. I left a few small comments.

one higher-level comment: it looks like the types don’t have descriptions for each property because they are defined as nested structures. I think VS Code will pick up descriptions in the type declarations file, which would be preferable. How difficult is that?

lib/espree.js Outdated
* } & SyntaxError} EnhancedSyntaxError
*/
/**
* We add `jsxAttrValueToken` ourselves.
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 100% sure, but I think this description might show up in VS Code Intellisense, so we should make sure the contents are fit for public consumption

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think they will only show up if one is introspecting the espree source, but to be safe, I've added a commit to keep the meta-comments separate from the typedefs.

lib/espree.js Outdated Show resolved Hide resolved
tests/lib/espree.js Show resolved Hide resolved
tools/js-for-ts.js Show resolved Hide resolved
@brettz9 brettz9 force-pushed the tsc-acorn2-jsdoc-eslint-parser branch from 086ef92 to 3c1bc7b Compare May 4, 2022 01:25
@brettz9 brettz9 force-pushed the tsc-acorn2-jsdoc-eslint-parser branch from 3c1bc7b to 4309e52 Compare May 4, 2022 01:25
@brettz9
Copy link
Contributor Author

brettz9 commented May 4, 2022

one higher-level comment: it looks like the types don’t have descriptions for each property because they are defined as nested structures. I think VS Code will pick up descriptions in the type declarations file, which would be preferable. How difficult is that?

Nesting can't be entirely avoided because that's where these properties occur--as child properties of an object parameter.

But yes, it is the case that how deeply the nested properties are defined will make a difference on how they are shown.

For example, this is how it is now in JSDoc:

image

...and how it shows up as a tooltip:

image

And after switching the nesting inline within the params in JSDoc ala:

image

...the tooltip looks like this:
image

But note that in even the latter case, the nested properties do not get the param descriptions added. The types are shown in more detail, but not so as to include the descriptions. The descriptions only appear for the top-level descriptions--in this case, the whole options object. Any description we'd want to appear would need to go on that whole object description.

But although we cannot apparently get the descriptions to show up, if you wanted to inline the properties as params as I did above, as you can see, it does get more type information to display, and I should be able to do that if you like.

Also:
- refactor: Can't use `@constructor` jsdoc in this situation, so refactor to class
Also:
- test: clarify test
@brettz9 brettz9 force-pushed the tsc-acorn2-jsdoc-eslint-parser branch from cec9310 to 0f1e2a1 Compare May 4, 2022 02:34
@brettz9

This comment was marked as outdated.

@nzakas
Copy link
Member

nzakas commented May 4, 2022

Fair enough. I suppose the nested properties are obvious enough that it doesn't matter too much.

Can you add a description of how this whole process works somewhere? If it's fairly short, it could go in the readme, or else a new file would be preferable. I can just imagine down the line as we're making changes that there will be questions.

@brettz9
Copy link
Contributor Author

brettz9 commented May 5, 2022

Note that I added an additional commit to convert to a single declaration file.

I reverted this just now upon testing against a more realistic npm install (via Github URL rather than npm link) and discovering that the paths were broken. Upon consulting the docs, I see it also mentions that with ESM imports one has to use an output directory, rather than a single output file.

I might also mention that although the code I've added substitutes local typedefs, it doesn't hide exports exported from one file for use by another. But AFAICT, with TypeScript using outDir, only the main file can be imported externally anyways.

@brettz9
Copy link
Contributor Author

brettz9 commented May 5, 2022

Fair enough. I suppose the nested properties are obvious enough that it doesn't matter too much.

Sure. But note that the current code doesn't convert the typedefs into inline params. I just did that to show our available options for nested display. If you want those nested options properties to show up on tooltips (albeit without descriptions), I'd need to modify the code to do so. It should be doable, but as it is now, it just looks like:

image

Can you add a description of how this whole process works somewhere? If it's fairly short, it could go in the readme, or else a new file would be preferable. I can just imagine down the line as we're making changes that there will be questions.

I've added a bit of an introduction to it on the helper project I created (and which this PR is now using) at https://github.com/es-joy/js2ts-assistant . I moved it to its own external project so that it could be reused in different projects. So as to avoid our redocumenting this within each project that uses it, is it sufficient for the main documentation to be hosted there?

espree.js Outdated
* @returns {Token[]} An array of tokens.
* @throws {SyntaxError} If the input code is invalid.
* @param {ParserOptions} options Options defining how to tokenize.
* @returns {acorn.Token[]|null} 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.

function tokenize returns Espree tokens. The main difference is that in Espree tokens type is a string, while in Acorn tokens type is an object.

Also, I'm not sure if this function can return null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While tokenize could return null, indeed in this function since it forces tokens: true, indeed it should not. I temporarily forced it as an acorn token, but still need to investigate the Espree token type returning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed now to indicate return of EspreeTokens.

espree.js Outdated
Comment on lines 74 to 126
this._regular = acorn.Parser.extend(espree());
const espreeParserFactory = espree();

// Cast the `acorn.Parser` to our own for required properties not specified in *.d.ts
this._regular = espreeParserFactory(/** @type {AcornJsxParser} */ (acorn.Parser));
Copy link
Member

Choose a reason for hiding this comment

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

Why can't we keep using acorn.Parser.extend? It's the official way to use Acorn plugins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TypeScript complains that the construct signatures are not compatible. For example, EspreeParser allows for null options, Acorn requires ecmaVersion now, etc. While one might think that Espree's looser requirements would allow Espree to be permitted here by TS, it is not.

Copy link
Member

Choose a reason for hiding this comment

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

TypeScript complains that the construct signatures are not compatible.

Does this mean that the types for acorn.Parser.extend are wrong, or that we were using acorn.Parser.extend in a way it wasn't actually intended to be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The types should be fine, and we were using it fine, but it was just easier to coerce the types.

I've added a commit to resume using the extend API.

Note that this change highlights one shortcoming in lib/espree.js. For the export:

export default () => {

    /**
     * Returns the Espree parser.
     * @param {AcornJsxParser} Parser The Acorn parser
     * @returns {typeof EspreeParser} The Espree parser
     */
    return Parser => {
      // ...
    };

...the @param should, I think, really be typeof acorn.Parser | AcornJsxParser since it can accept a plain Acorn parser as well as a JSX one.

If we did this, we should be able to drop the unknown conversion (since the plain Acorn parser would be known to be acceptable), and it seems to fit more semantically.

However, this introduces a whole can of worms in that, with a union of these two class signatures, TypeScript no longer properly finds the various Acorn base parser properties (despite the fact that either class gives them), and we need to inform TS of their existence by indicating a great deal of them explicitly in the constructor.

Another option might be to just accept typeof acorn.Parser as the param, but then TS complains about the JSX-specific properties being optionally used within the class, and adding duck-checking before their use doesn't appear to satisfy TS to signal that the properties do not always exist (and don't need to exist).

Or, we could just leave it as is.

@brettz9 brettz9 force-pushed the tsc-acorn2-jsdoc-eslint-parser branch from ed0a6c7 to b4e7aa7 Compare May 9, 2022 08:15
dist/espree.d.ts Outdated Show resolved Hide resolved
@brettz9 brettz9 force-pushed the tsc-acorn2-jsdoc-eslint-parser branch from 44a41e0 to 4680aec Compare May 11, 2022 00:18
@nzakas
Copy link
Member

nzakas commented May 12, 2022

@brettz9 it looks like that repo is private? Is there anything specific to Espree that needs documenting?

@brettz9
Copy link
Contributor Author

brettz9 commented May 12, 2022

@brettz9 it looks like that repo is private?

Oops, sorry. I guess organizations get private repos by default. Fixed to be public.

Is there anything specific to Espree that needs documenting?

in the /tools/js-for-ts.js file, I added a few further comments as to the arguments we're supplying there.

@nzakas
Copy link
Member

nzakas commented Jun 2, 2022

@brettz9 what is the status of this PR?

@brettz9
Copy link
Contributor Author

brettz9 commented Jun 2, 2022

We are currently blocked by this acorn-jsx PR, though there was progress recently in getting a review by someone with more experience with TypeScript, who is now satisfied with its status.

The one area I think that is left on my end is to do some more testing as to whether we need separate declaration files for ESM and CJS builds. While I've confirmed the config appears sufficient for working in either environment as far as type-aware IDE tooltips, and successful, error-free compiling of JavaScript files, I should ensure that builds are successfully made from any TypeScript source files which use our declaration files.

I should be more free to take a look after this weekend.

@brettz9
Copy link
Contributor Author

brettz9 commented Jun 10, 2022

Turns out re: the class issue, that we don't need to export a bogus class (we have to export two typedefs--one for the parser constructor/static and one for the instance), so I'm working on that now as I have energy.

Have a small blocker for that now though with a dependency jsdoc-type-pratt-parser, but the maintainer generally responds after not too long.

I also still have to confirm the module styles work well with TypeScript files targeting the espree package.

@nzakas
Copy link
Member

nzakas commented Jun 10, 2022

Sounds good. Take your time, rest up, this will be here when you're ready to finish it up.

@nzakas
Copy link
Member

nzakas commented Mar 7, 2023

Closing for now as we aren't continuing on with this.

@nzakas nzakas closed this Mar 7, 2023
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.

4 participants