eslint / espree

An Esprima-compatible JavaScript parser
BSD 2-Clause "Simplified" License
2.26k stars 189 forks source link

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

Closed brettz9 closed 1 year ago

brettz9 commented 2 years ago

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:

brettz9 commented 2 years ago

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 commented 2 years ago

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.

brettz9 commented 2 years ago

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

nzakas commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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?

nzakas commented 2 years ago

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

brettz9 commented 2 years ago

@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 commented 2 years ago

@brettz9 what is the status of this PR?

brettz9 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

nzakas commented 1 year ago

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