Skip to content

Better type definitions #946

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

Closed
djcsdy opened this issue Apr 6, 2020 · 11 comments
Closed

Better type definitions #946

djcsdy opened this issue Apr 6, 2020 · 11 comments

Comments

@djcsdy
Copy link

djcsdy commented Apr 6, 2020

There is a much better set of type definitions for acorn on DefinitelyTyped: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/acorn/index.d.ts

Normally, in a project that consumes acorn, it would be easy to use the above type definitions by adding a dependency on @types/acorn. However, acorn distributes its own type definitions and those take precedence.

The type definitions on DefinitelyTyped give a much better experience because they reuse the existing type definitions for estree. These include proper type definitions for all the subtypes of Node. Without these it's necessary to do a lot of typecasting and the types become pretty much useless.

Please consider either removing the type definitions from acorn, or replacing them with the above type definitions from DefinitelyTyped. In either case it will be necessary to also update the type definitions for acorn-walk.

I would be happy to make a PR for this if you let me known which is your preferred approach.

@marijnh
Copy link
Member

marijnh commented Apr 6, 2020

Those definitions directly return ESTree node types, which are missing properties that are present on Acorn's output. As such, though they are indeed more refined and complete, they aren't a great match for Acorn's interface either.

@djcsdy
Copy link
Author

djcsdy commented Apr 6, 2020

OK.

Sounds like the best approach is to improve acorn's type definitions so that they're in line with the ESTree type definitions, but with acorn's additional properties added. Would you be interested in a PR for that?

@marijnh
Copy link
Member

marijnh commented Apr 6, 2020

Yes, it'd be wonderful if someone would take a serious look at the type defs—they haven't been a priority for me so far, and I'm aware that this is showing. It may even be possible to use some advanced TypeScript wizardry to inject our additional properties into the ESTree types without too much extra syntactic noise, but I haven't found a way a to do that.

@RReverser
Copy link
Member

It may even be possible to use some advanced TypeScript wizardry to inject our additional properties into the ESTree types without too much extra syntactic noise

This is actually pretty cool idea and certainly should be possible. (FWIW I wrote the type definitions on DefinitelyTyped referenced above, at the same time as I added ESTree definitions back in 2015.)

@RReverser
Copy link
Member

A quick attempt that seems to work fairly well (although the pretty-printed types on hover might need some further improvements): playground

@djcsdy
Copy link
Author

djcsdy commented Apr 7, 2020

I will take a look at this as soon as I get a chance. Or let me know if you'd rather do it @RReverser :-).

@threkk
Copy link

threkk commented Jun 7, 2020

Sorry for disturbing but is there any update on this issue?

@djcsdy
Copy link
Author

djcsdy commented Jun 7, 2020

Speaking only for myself, not for maintainers of this project: it’s on my todo list but other projects are taking priority for me for the foreseeable future. If someone else wants to take this on please drop a note here so I know not to tread on your toes.

@swyxio
Copy link

swyxio commented Aug 7, 2020

hello! just ran into these exact issues and figured i might take a crack at it. i'm just not clear about the scope of work - is it as broad as "just upgrade acorn's types to incorporate all the DefinitelyTyped's contributions"? I'm also unclear how you want to be injecting the ESTree stuff as well. let me know what clear intentions you have, and what is still an open qtn, and i could take a crack at it

@RReverser
Copy link
Member

@sw-yx I described a way that would work above. Ideally we should use ESTree types with our extensions.

@marijnh
Copy link
Member

marijnh commented Sep 21, 2023

Attached patch merges @rwalle 's work on new types. We decided that there were too many differences between @types/estree and what Acorn is actually doing to make integrating with those types practical, so the new type definitions define the entire node hierarchy locally.

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

No branches or pull requests

6 participants
@marijnh @djcsdy @threkk @RReverser @swyxio and others