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

@types for espree? #529

Open
srijan-paul opened this issue Jan 12, 2022 · 31 comments
Open

@types for espree? #529

srijan-paul opened this issue Jan 12, 2022 · 31 comments
Labels

Comments

@srijan-paul
Copy link

Currently I do not see any way of using the espree npm package in a typescript project.
I think the API is simple enough that .d.ts files can be provided with some effort.

I'm curious if such declarations already exist.
If not, I'd be more than happy to contribute them myself.

@nzakas
Copy link
Member

nzakas commented Jan 12, 2022

While Espree itself has a fairly simple API, the AST that is returned is quite involved. Would it be necessary to get type definitions for all of the AST nodes too?

@srijan-paul
Copy link
Author

Would it be necessary to get type definitions for all of the AST nodes too?

I believe so, yeah.
That way working with the AST in typescript becomes convenient, like with typescript-eslint-parser's AST.
What do you think?

@nzakas
Copy link
Member

nzakas commented Jan 13, 2022

I think it's a good idea, but my concern is always about maintainability. I don't think checking in a separate .d.ts file that we have to manually update is a good idea. If we can make it automatic, then I think it's worth pursuing. I've done some experimenting with tsc and JSDoc that can probably handle the Espree-specific stuff:
https://humanwhocodes.com/snippets/2020/10/create-typescript-declarations-from-javascript-jsdoc/

It looks like there's a DefinitelyTyped file for ESTree, which should cover the AST bit:
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/estree/index.d.ts

If you're interesting in investigating this, we can definitely consider it.

@srijan-paul
Copy link
Author

srijan-paul commented Jan 14, 2022

If we can make it automatic, then I think it's worth pursuing

Yeah, I don't think it's that common to write .d.ts files by hand - except for some cases where people tend to simulate dependent types (to some limited extent). I think it would be great if we could have them laid out such that the properties of a node can be inferred if the type is known. For example:

function foo(node: Node) {
  if (node.type === 'CallExpression') {
     // Accessing the property `callee` without casting `node` to the `CallExpression` type should not throw
     // a typescript compiler error.
    console.log(node.callee);
  }
}

As for the API, I think JSDoc should take care of it.
What do you think?

@nzakas
Copy link
Member

nzakas commented Jan 14, 2022

I think if we can make all of this automatic, then we can include it in the Espree package during each release so folks don't have to go through the trouble of downloading a separate package. :)

I think it would be great if we could have them laid out such that the properties of a node can be inferred if the type is known.

If there's an easy way to do that without manual intervention, that sounds like a bonus.

It sounds like you have a good idea of how this could work, so if you want to prototype something up for feedback, we can go from there.

Thanks!

@srijan-paul
Copy link
Author

It sounds like you have a good idea of how this could work, so if you want to prototype something up for feedback, we can go from there.

Yep, I'll open a PR in about a week or so and lets see if it works out.

@nzakas
Copy link
Member

nzakas commented Jan 18, 2022

Awesome, thank you!

@srijan-paul
Copy link
Author

#530 should help with TS bindings for parts of the API.

@brettz9
Copy link
Contributor

brettz9 commented Feb 7, 2022

It looks like there's a DefinitelyTyped file for ESTree, which should cover the AST bit: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/estree/index.d.ts

@nzakas : Do you think that eslint-visitor-keys should, in place of manually maintaining its own listing, parse this @types/estree AST (as with @typescript-eslint/parser), from which espree derives (more clearly so after #532 ) and build its own listing therefrom, allowing for more automated updates to that package (especially as it looks like that declaration file is the best thing as far as a formal technical authority given the apparent lack of interest in ESTree itself handling it per estree/estree#256 (comment) )?

@nzakas
Copy link
Member

nzakas commented Feb 8, 2022

That is a fascinating idea. anything we can do to automate types sounds good to me.

@brettz9
Copy link
Contributor

brettz9 commented Feb 8, 2022

Ok, great--I can look into adding that.

@brettz9
Copy link
Contributor

brettz9 commented Feb 12, 2022

The job will be simplified significantly if we can base the types on acorn and acorn-jsx's types.

For Acorn, I submitted acornjs/acorn#1104 which has been merged but not yet released. Also submitted acornjs/acorn#1105 on which I am hoping for a merge.

For acorn-jsx, I submitted acornjs/acorn-jsx#130 though noting that it could really benefit from someone more experienced with TypeScript taking a look, particularly for the changes it seems I had but no choice to make to the namespace/module so it could work from both CJS and ESM. If anyone here could take a look, that might also, I imagine, reassure the maintainer to potentially advance the PR.

With these changes, I have a local branch passing tests (when linked to my forks) and which includes checkJs: true.

@brettz9
Copy link
Contributor

brettz9 commented Feb 12, 2022

Also, besides microsoft/TypeScript#46011 causing some ugliness within the JSDoc in not being able to use typedefs to shorten type references, one even worse hack is currently apparently necessary for us to take a 100% pure JSDoc-based solution while referencing a class type that we export. See microsoft/TypeScript#22126 (comment) for details.

We could get around this by deviating in one case from the goal to use inline JSDoc only, and from our JSDoc typedefs, import a small internal TypeScript declaration file (just a few lines) which only has the class export. This would still allow us to build a declaration file based mostly on JSDoc only while avoiding the great ugliness of the above-mentioned hack (requiring including a dummy class and methods in source).

@nzakas
Copy link
Member

nzakas commented Feb 12, 2022

We could get around this by deviating in one case from the goal to use inline JSDoc only, and from our JSDoc typedefs, import a small internal TypeScript declaration file (just a few lines) which only has the class export.

I’m not opposed to this. My main concern is that I don’t want to have a separate file that we need to remember to update whenever there’s a change on the JS file. Could we perhaps generate the TS file from the JS definition of Parser?

@brettz9
Copy link
Contributor

brettz9 commented Feb 12, 2022

We could get around this by deviating in one case from the goal to use inline JSDoc only, and from our JSDoc typedefs, import a small internal TypeScript declaration file (just a few lines) which only has the class export.

I’m not opposed to this. My main concern is that I don’t want to have a separate file that we need to remember to update whenever there’s a change on the JS file.

Sure, and a worthy goal I think. Hopefully TS may better accommodate this use case for plain JS users.

Could we perhaps generate the TS file from the JS definition of Parser?

If you mean parsing the AST for the JS file, yes, that should be doable with some work. I expect it would be assisted by using https://github.com/es-joy/jsdoc-eslint-parser to get parsed JSDoc AST from the JSDoc blocks.

@nzakas
Copy link
Member

nzakas commented Feb 15, 2022

I just had an idea: can we simplify this by letting TypeScript generate the declarations file and then just removing the definitions we don’t want? Seems like a fairly straightforward small JS script would do the trick?

@brettz9
Copy link
Contributor

brettz9 commented Feb 15, 2022

If you mean for the typedefs, then I would think so and can look into it. But this wouldn't solve our problem with the exported class because that is the case of a definition that is missing. But for the typedefs I think it is a good solution, but just with the disadvantage that it might be a little unexpected by developers used to it generating exports.

@nzakas
Copy link
Member

nzakas commented Feb 16, 2022

But this wouldn't solve our problem with the exported class because that is the case of a definition that is missing.

Right, forgot about that part. In any event, as long as we have an automated solution, I’ll be happy.

@brettz9
Copy link
Contributor

brettz9 commented Feb 18, 2022

We might be able to do the following, though noting we'd have to build and lint a temporary directory I think for TS to use that.

  1. Add a @private tag in the same block as the @typedef as proposed at Support for opting out of jsdoc @typedef exports microsoft/TypeScript#46011 (comment) and use that knowledge to expand, in a preprocessing step, the other @typedef's or @type's within the page referencing the private item.
  2. Add an @export tag to the class as raised at Add support for an @export tag microsoft/TypeScript#22126 , and use that to determine that a global dummy class ought to be temporarily built for use in validating the JavaScript and building the definition file.

The advantage of this particular preprocessing rather than postprocessing approach is that it could be reuseable across projects until such time as TypeScript may get on board or provide an alternative. We might still need postprocessing though to ensure that the class export needed within the project to indicate the type of class being passed around is not intended to be exported by the library (except as a type if it is possible in TS to export the class type without implying a class implementation) because we won't actually be exposing the actual class directly.

@nzakas nzakas linked a pull request Feb 19, 2022 that will close this issue
@nzakas
Copy link
Member

nzakas commented Mar 10, 2022

Sorry I lost track of this. This is warping my mind a bit. I’d say let’s go with what you think is the best approach at this point. Getting something working is better than trying to figure out the perfect choice.

@brettz9
Copy link
Contributor

brettz9 commented Mar 11, 2022

Sorry I lost track of this.

No worries. Have needed some rest anyhow.

This is warping my mind a bit. I’d say let’s go with what you think is the best approach at this point. Getting something working is better than trying to figure out the perfect choice.

Sure. FWIW, I've set up a fork of escodegen and estraverse in my "es-joy" organization. I know you hadn't wanted to maintain certain extra tools that could become a headache to maintain, but if you end up needing these pretty core tools, and with reviews not frequent on the forked project, welcome to join or send in PRs.

Anyways, I hope to use these together with jsdoc-eslint-parser (and esquery) to modify the JSDoc blocks and add to the JavaScript source before supplying it to TS. With the libraries I wanted to use now set up, I should at least start to get some momentum on this, though as it is new terrain, I can't speak for sure as to how quickly.

@nzakas
Copy link
Member

nzakas commented Mar 12, 2022

Sounds good! No rush at all.

@brettz9
Copy link
Contributor

brettz9 commented Apr 11, 2022

I've been concentrating on the type checking side of things, so haven't gotten to review the PR, @srijan-paul but it had looked fine earlier outside of my one comment.

FWIW, @nzakas , I've submitted babel/babel#14445 and babel/babel#14449 over with Babel, being as it seems their offering of tools for manipulation of AST seems it would be the smoothest route toward implementing the full type checking. If not, or in the interim, I think manipulation should be still doable without actually needing to retool escodegen or such, as it seems we can convert from estree to babel to take advantage of @babel/traverse AST manipulation methods and use @babel/generate to serialize (even while initially parsing and working with ESTree type AST).

@nzakas
Copy link
Member

nzakas commented Apr 13, 2022

@brettz9 thanks for the update. Just so we are all on the same page, can you summarize the main problems you are trying to solve? I’m just having a hard time parsing it out of this thread. I think we were concerned with:

  1. An internal-only type being exposed publicly
  2. ???

Anything else?

@brettz9
Copy link
Contributor

brettz9 commented Apr 13, 2022

Yes:

  1. Internal-only types being exposed publicly
  2. A class type that needs to be exported for discovery by other files

Btw, I'm not sure Babel will be enough here, as I'm not certain whether their modification functions--which can be used during traversal only apparently--cover comments.

What would help considerably I think is we had a library to be given a Node, and after changing a value on the node (or removing the node or adding to one), the library could determine what the node's new range offsets should be based on the length of the newly serialized form (e.g., if a JSDoc tag line is removed, take into account what how much space the new comment should take) and then adjust all of the other ranges in the document to make room (or condense room) for the modified, removed, or added node.

@nzakas
Copy link
Member

nzakas commented Apr 14, 2022

What was the specific example for number 2 again?

What would help considerably I think is we had a library to be given a Node, and after changing a value on the node (or removing the node or adding to one), the library could determine what the node's new range offsets should be based on the length of the newly serialized form (e.g., if a JSDoc tag line is removed, take into account what how much space the new comment should take) and then adjust all of the other ranges in the document to make room (or condense room) for the modified, removed, or added node.

Since this wouldnt be executed at runtime, an easier option would be to take the source code, make your change to the text based on the node range, and then re-parse the result text to get updated location information.

I’m not sure this narrow use case is worth creating a whole new set of AST tools. :)

@brettz9
Copy link
Contributor

brettz9 commented Apr 14, 2022

What was the specific example for number 2 again?

Here's the class I had to have auto-generated: https://github.com/brettz9/espree/blob/6abea2010ed26e0f520f0720d316ce4080016376/lib/espree.js#L25-L58

...in order to export this class' type:

https://github.com/brettz9/espree/blob/6abea2010ed26e0f520f0720d316ce4080016376/lib/espree.js#L200-L542

What would help considerably I think is we had a library to be given a Node, and after changing a value on the node (or removing the node or adding to one), the library could determine what the node's new range offsets should be based on the length of the newly serialized form (e.g., if a JSDoc tag line is removed, take into account what how much space the new comment should take) and then adjust all of the other ranges in the document to make room (or condense room) for the modified, removed, or added node.

Since this wouldnt be executed at runtime, an easier option would be to take the source code, make your change to the text based on the node range, and then re-parse the result text to get updated location information.

I’m not sure this narrow use case is worth creating a whole new set of AST tools. :)

To fix this one issue, sure, but this is the kind of tool I think we could use in all kinds of ESLint rules and not only for comments, and seems a good occasion to force myself to make some progress on it. I know you mentioned elsewhere that it was a difficult issue to solve, including with the problem that there are different parsers that get consumed by ESLint, but I can only think that it needs to be worked on, no?

The changes to escodegen are not very significant; it's really I think that we're missing this one kind of "CRISPR for AST" tool in the ecosystem (and I probably need to get my parser to settle on a point of attachment for the comment AST which I think I can do merely based on where the comment happens to fall even if there are multiple valid points of attachment).

@nzakas
Copy link
Member

nzakas commented Apr 16, 2022

To fix this one issue, sure, but this is the kind of tool I think we could use in all kinds of ESLint rules and not only for comments, and seems a good occasion to force myself to make some progress on it.

I worry that the scope of this work has grown beyond the immediate benefit of solving the problem at hand. I hear you that this is an interesting realm of problem, and perhaps there is a tooling gap that would be nice to fill, but it seems like this particular issue can be solved with a simpler solution.

I’d much rather solve this issue with the simpler approach first, rather than invest in a more complicated approach that may or may not be useful in other places. (We really don’t know.)

I know you mentioned elsewhere that it was a difficult issue to solve, including with the problem that there are different parsers that get consumed by ESLint, but I can only think that it needs to be worked on, no?

ESLint has been around for eight years without solving this problem, so I don’t think it’s a big missing piece. I’m also highly skeptical that any kind of AST mutation can be safely done during linting due to the up-and-down traversal coupled with rules managing their own state of what they’ve already seen. I’m not saying it’s impossible, but I think it’s a stretch to think creating Babel-like AST manipulation utilities will make much difference in what we can do without a fundamental redesign of ESLint’s architecture.

And as already mentioned, I don’t think this issue is a good enough reason to dig into that amount of work.

@linonetwo
Copy link

Is there a typing already? TS is asking for it.

图片

pnpm i --save-dev @types/espree doesn't work.

linonetwo added a commit to tiddly-gittly/TidGi-Desktop that referenced this issue Jun 8, 2023
use this .d.ts workaround until eslint/js#529 fixed
@nzakas
Copy link
Member

nzakas commented Jun 8, 2023

No there isn’t.

linonetwo added a commit to tiddly-gittly/TidGi-Desktop that referenced this issue Jun 9, 2023
use this .d.ts workaround until eslint/js#529 fixed
@stuft2
Copy link

stuft2 commented Aug 16, 2023

This get's us 90% of the way there imo: tiddly-gittly/TidGi-Desktop@c77d9fe

Thanks @linonetwo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Implementing
Development

Successfully merging a pull request may close this issue.

5 participants