Improve typings: ESM, `AcornJsxParser` class and `tokTypes` const

[brettz9]

As explained at https://github.com/acornjs/acorn/pull/1104 , we're looking to add type support to Espree (and other projects in the ESLint space), and in the case of your project, it looks like we could use this missing class and ESM import ability (especially since limiting ourselves to basic JSDoc for the TS declaration file generation makes it more useful not to have to reimplement types ourselves).

I'm pretty new to using TypeScript, so hope I've gotten things done all right. It seems I needed to remove the export = jsx and instead use export default jsx; to support ESM, but hopefully someone more competent in TS can give it a good look.

Also, the PR doesn't cover all methods or properties, but I hope this will at least improve things (and at least meets our immediate needs).

Best wishes...

[brettz9]

I've now added missing properties and methods as well as fixed the return value of the constructor to give typeof AcornJsxParser instead of typeof acorn.Parser

[brettz9]

Any idea on chances of being able to give a review? Thanks...

[RReverser]

Hey, sorry, following the invasion of my home country I wasn't on Github much nor in the right mindset for reviews.

From a quick look this seems reasonable, although I'm a bit rusty. Did you have a chance to try this on a real-world project or anyone else to look at this, just to be sure?

[brettz9]

My sympathies for such severe difficulties, and very understandable regarding reviews.

I haven't been able to get others to look at it, and I don't have real world uses outside of use within the espree parser (which uses the types internally), but I have confirmed that those additions which are relevant to our project are correctly helping our project enforce the expected types (and they do not do so otherwise). The expected types also show up in my TS-aware IDE (Atom with atom-typescript), whether requiring or importing.

[brettz9]

Just one point I might add--Acorn has merged some updates to some other type changes I offered and released as part of v8.7.1 , so technically the types would require an update of acorn to that version to work as expected, though I expect given your broad peerDependencies support, that you wouldn't want to bump those versions. But just letting you know (that such a version of Acorn is needed for proper TS support).

[brettz9]

@remcohaszing : Having contributed the original TS declaration file, do you think you could take a look at these additions/changes to give an extra confirmation here about whether the type changes should work as expected?

[brettz9]

Now that I've applied most of your fixes, here's what I think is left of mine differing from yours, @remcohaszing :

I made fiddled a bit based on these changes and I believe the following types are correct:

declare function jsx(options?: jsx.Options): (BaseParser: typeof acorn.Parser) => new () => jsx.AcornJsxParser

declare namespace jsx {

<...SNIP...>

  export const tokTypes: {
    jsxName: acorn.TokenType,
    jsxText: acorn.TokenType,
    jsxTagEnd: acorn.TokenType,
    jsxTagStart: acorn.TokenType
  } & typeof acorn.tokTypes

For mostly aesthetic rather than functional reasons, I changed this to be under one object:

declare const jsx: {
  tokTypes: typeof acorn.tokTypes;
  (options?: jsx.Options): (BaseParser: typeof acorn.Parser) => jsx.AcornJsxParser
}

It seemed to me that putting tokTypes visibly on the same object clears up any confusion some like myself might have in that tokTypes was not a named export. But again, I realize this is probably just my preference.

Regarding AcornJsxParser, there are two ways in which mine differs:

1. I don't think new () without any arguments would work here, as acorn-jsx reuses acorn's constructor which has its own parameter type expectations.
2. I've split off the constructor and interface I think by necessity.

For the constructor type, I have this:

  export interface AcornJsxParser extends Pick<P, keyof P> {
    readonly acornJsx: {
      tokTypes: TokTypes;
      tokContexts: TokContexts
    };

    new (options: acorn.Options, input: string, startPos?: number): IAcornJsxParser;
  }

Unless defining a class--which we don't want to do since we want to export the type but not suggest a class is being exported--this was I think necessary because:

1. With interfaces, TypeScript expects separate types for the constructor and for the instance interface. The static property acornJsx cannot be defined on the instance type as it is actually on the constructor type.
2. We still need the static acorn property from the Acorn Parser. To get this extension, I tried it with just extends P:

   type P = typeof acorn.Parser;
   export interface AcornJsxParser extends P {
   // <SNIP>
   new (options: acorn.Options, input: string, startPos?: number): IAcornJsxParser;  
   }

   ...but TypeScript complained for some reason that P and IAcornJsxParser were not the same base type (when one extends something, one would think a different type would be expected, but I guess interface constructor extending is normally expected to just add some static properties).

In any case, we do to all appearances need a separate IAcornJsxParser interface since we're creating instances and instances distinct from acorn's Parser. But though I couldn't get this by extending it with Acorn's Parser directly, I added the extends Pick<P, keyof P> so as to ensure we were grabbing the static acorn property off of acorn's Parser (and any other static properties that might be added in the future). We could thus get all the static properties along with indicating the creation of our own separate interface, IAcornJsxParser.

Then there is the more straightforward instance interface itself:

  export interface IAcornJsxParser extends acorn.Parser {
    jsx_readToken(): string;
    <SNIP>

[remcohaszing]

The types are correct now. I just added some comments on some stylistic nits.

[brettz9]

I've applied the stylistic changes. There is, per your question, indeed, one non-stylistic issue remaining, however (re: jsx.TokTypes).

[brettz9]

@RReverser : With an additional review (by @remcohaszing) having been done, are you ok to merge?

[RReverser]

Actually, sorry - one question - I understand the need to expose token types, but can you explain why the jsx_* methods need to be exposed in the typings? They're kind of implementation details, does something in espree depend on accessing those methods directly?

[brettz9]

espree is extending the acorn or acorn-jsx parser class. Unfortunately, the branch I'm working on is in an unstable state atm, so I can't confirm now that those methods were necessary. I seem to recall at least some were, but I could be mistaken.

[RReverser]

I suppose there isn't much harm - acorn-JSX remained pretty much unchanged for years - just slightly worried about exposing implementation details as typing become part of semver.

[RReverser]

Given that it hasn't changed in a while, I suppose this is fine.