feat: Programmatic building of type-checkable JS and declaration files

[brettz9]

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]

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.

[mdjermanovic]

Suggested change

	* allowReserved?: boolean | "never",
	* allowReserved?: boolean,

Only true and false are allowed values.

[mdjermanovic]

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

[brettz9]

Fixed, thanks.

[mdjermanovic]

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

[brettz9]

Fixed.

[brettz9]

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

[mdjermanovic]

These are the only options passed to underlying parsers:

espree/lib/espree.js

Lines 69 to 79 in 6c718af

	super({
	// do not use spread, because we don't want to pass any unknown options to acorn
	ecmaVersion: options.ecmaVersion,
	sourceType: options.sourceType,
	ranges: options.ranges,
	locations: options.locations,
	allowReserved: options.allowReserved,
	// Truthy value is true for backward compatibility.
	allowReturnOutsideFunction: options.allowReturnOutsideFunction,

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

[mdjermanovic]

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.

[nzakas]

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?

[nzakas]

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

[brettz9]

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.

[brettz9]

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:

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

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

...the tooltip looks like this:

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.

[nzakas]

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]

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]

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:

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?

[mdjermanovic]

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?

[brettz9]

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.

[brettz9]

Fixed now to indicate return of EspreeTokens.

[mdjermanovic]

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

[brettz9]

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.

[mdjermanovic]

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?

[brettz9]

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.

[nzakas]

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

[brettz9]

@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]

@brettz9 what is the status of this PR?

[brettz9]

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]

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]

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

[nzakas]

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