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

Improve typings: ESM, AcornJsxParser class and tokTypes const #130

Merged
merged 19 commits into from
Nov 14, 2022

Conversation

brettz9
Copy link
Contributor

@brettz9 brettz9 commented Feb 10, 2022

As explained at acornjs/acorn#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 brettz9 changed the title feat: Support ESM declaration imports; add an AcornJsxParser class feat: Support ESM declaration imports; add an AcornJsxParser class and tokTypes const Feb 12, 2022
brettz9 added a commit to brettz9/espree that referenced this pull request Feb 12, 2022
0. depends on acorn and acorn-jsx being updated per PRs
    a. acornjs/acorn#1104 (merged but not yet released)
    b. acornjs/acorn#1105
    c. acornjs/acorn-jsx#130
1. Refactor to avoid `typedef` shortcuts when not desired for export
2. Avoid dummy class, so parse JavaScript with https://github.com/es-joy/jsdoc-eslint-parser

feat: Adds JSDoc-based TypeScript declaration file

Also:
1. chore: adds `editorconfig`
2. refactor: Removes unused esprima
3. refactor: changes to force EspreeParser constructor to convert `new String` to plain string (for typing)
4. refactor: switches to `Object.keys` to avoid `hasOwnProperty` and easier for typing
5. refactor: drops a use of `acorn.Parser.extend` for typing purposes
6. refactor: checks for existence of `tokens` in `tokenize` (as may be absent)
7. refactor: checks for existence of `firstNode.range` and `firstNode.loc`  in `parse` (as may be absent)
8. refactor: checks for existence of `extra.lastToken.range` and `extra.lastToken.loc` in `parse` (as may be absent)
7. feat: throws specific error if `jsx_readString` superclass undefined
8. refactor: checks for existence of `lastTemplateToken.loc` and `lastTemplateToken.range` in `token-translator.js` (as may be absent)
index.d.ts Outdated
jsx_readString(quote: number): void;
}

export as namespace jsx;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, TIL this works.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This declares jsx is available as a global namespace, which is incorrect.

brettz9 added a commit to brettz9/espree that referenced this pull request Feb 12, 2022
0. depends on acorn and acorn-jsx being updated per PRs
    a. acornjs/acorn#1104 (merged but not yet released)
    b. acornjs/acorn#1105 (merged but not yet released)
    c. acornjs/acorn-jsx#130
1. Refactor to avoid `typedef` shortcuts when not desired for export
2. Avoid dummy class, so parse JavaScript with https://github.com/es-joy/jsdoc-eslint-parser

feat: Adds JSDoc-based TypeScript declaration file

Also:
1. chore: adds `editorconfig`
2. refactor: Removes unused esprima
3. refactor: changes to force EspreeParser constructor to convert `new String` to plain string (for typing)
4. refactor: switches to `Object.keys` to avoid `hasOwnProperty` and easier for typing
5. refactor: drops a use of `acorn.Parser.extend` for typing purposes
6. refactor: checks for existence of `tokens` in `tokenize` (as may be absent)
7. refactor: checks for existence of `firstNode.range` and `firstNode.loc`  in `parse` (as may be absent)
8. refactor: checks for existence of `extra.lastToken.range` and `extra.lastToken.loc` in `parse` (as may be absent)
7. feat: throws specific error if `jsx_readString` superclass undefined
8. refactor: checks for existence of `lastTemplateToken.loc` and `lastTemplateToken.range` in `token-translator.js` (as may be absent)
@brettz9
Copy link
Contributor Author

brettz9 commented Feb 12, 2022

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 added a commit to brettz9/espree that referenced this pull request Feb 12, 2022
0. depends on acorn and acorn-jsx being updated per PRs
    a. acornjs/acorn#1104 (merged but not yet released)
    b. acornjs/acorn#1105 (merged but not yet released)
    c. acornjs/acorn-jsx#130
1. Refactor to avoid `typedef` shortcuts when not desired for export
2. Avoid dummy class, so parse JavaScript with https://github.com/es-joy/jsdoc-eslint-parser

feat: Adds JSDoc-based TypeScript declaration file

Also:
1. chore: adds `editorconfig`
2. refactor: Removes unused esprima
3. refactor: changes to force EspreeParser constructor to convert `new String` to plain string (for typing)
4. refactor: switches to `Object.keys` to avoid `hasOwnProperty` and easier for typing
5. refactor: drops a use of `acorn.Parser.extend` for typing purposes
6. refactor: checks for existence of `tokens` in `tokenize` (as may be absent)
7. refactor: checks for existence of `firstNode.range` and `firstNode.loc`  in `parse` (as may be absent)
8. refactor: checks for existence of `extra.lastToken.range` and `extra.lastToken.loc` in `parse` (as may be absent)
7. feat: throws specific error if `jsx_readString` superclass undefined
8. refactor: checks for existence of `lastTemplateToken.loc` and `lastTemplateToken.range` in `token-translator.js` (as may be absent)
@brettz9
Copy link
Contributor Author

brettz9 commented Mar 9, 2022

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

brettz9 added a commit to brettz9/espree that referenced this pull request Mar 10, 2022
0. depends on acorn and acorn-jsx being updated per PRs
    a. acornjs/acorn#1104 (merged but not yet released)
    b. acornjs/acorn#1105 (merged but not yet released)
    c. acornjs/acorn-jsx#130
1. Refactor to avoid `typedef` shortcuts when not desired for export
2. Avoid dummy class, so parse JavaScript with https://github.com/es-joy/jsdoc-eslint-parser

feat: Adds JSDoc-based TypeScript declaration file

Also:
1. chore: adds `editorconfig`
2. refactor: Removes unused esprima
3. refactor: changes to force EspreeParser constructor to convert `new String` to plain string (for typing)
4. refactor: switches to `Object.keys` to avoid `hasOwnProperty` and easier for typing
5. refactor: drops a use of `acorn.Parser.extend` for typing purposes
6. refactor: checks for existence of `tokens` in `tokenize` (as may be absent)
7. refactor: checks for existence of `firstNode.range` and `firstNode.loc`  in `parse` (as may be absent)
8. refactor: checks for existence of `extra.lastToken.range` and `extra.lastToken.loc` in `parse` (as may be absent)
7. feat: throws specific error if `jsx_readString` superclass undefined
8. refactor: checks for existence of `lastTemplateToken.loc` and `lastTemplateToken.range` in `token-translator.js` (as may be absent)
brettz9 added a commit to brettz9/espree that referenced this pull request Apr 21, 2022
0. depends on acorn and acorn-jsx being updated per PRs
    a. acornjs/acorn#1104 (merged but not yet released)
    b. acornjs/acorn#1105 (merged but not yet released)
    c. acornjs/acorn-jsx#130
1. Refactor to avoid `typedef` shortcuts when not desired for export
2. Avoid dummy class, so parse JavaScript with https://github.com/es-joy/jsdoc-eslint-parser

feat: Adds JSDoc-based TypeScript declaration file

Also:
1. chore: adds `editorconfig`
2. refactor: Removes unused esprima
3. refactor: changes to force EspreeParser constructor to convert `new String` to plain string (for typing)
4. refactor: switches to `Object.keys` to avoid `hasOwnProperty` and easier for typing
5. refactor: drops a use of `acorn.Parser.extend` for typing purposes
6. refactor: checks for existence of `tokens` in `tokenize` (as may be absent)
7. refactor: checks for existence of `firstNode.range` and `firstNode.loc`  in `parse` (as may be absent)
8. refactor: checks for existence of `extra.lastToken.range` and `extra.lastToken.loc` in `parse` (as may be absent)
7. feat: throws specific error if `jsx_readString` superclass undefined
8. refactor: checks for existence of `lastTemplateToken.loc` and `lastTemplateToken.range` in `token-translator.js` (as may be absent)
@RReverser
Copy link
Member

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
Copy link
Contributor Author

brettz9 commented Apr 28, 2022

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
Copy link
Contributor Author

brettz9 commented Apr 28, 2022

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
Copy link
Contributor Author

brettz9 commented May 5, 2022

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

Copy link
Contributor

@remcohaszing remcohaszing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this addition! I believe it’s very valuable to add these types, but I did find some incorrect type definitions.

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

import * as acorn from 'acorn';

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

declare namespace jsx {
  export interface Options {
    allowNamespacedObjects?: boolean;
    allowNamespaces?: boolean;
  }

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

  export type TokContexts = {
    tc_oTag: acorn.TokContext,
    tc_cTag: acorn.TokContext,
    tc_expr: acorn.TokContext
  };

  export interface AcornJsxParser extends acorn.Parser {
    readonly acornJsx: {
      tokTypes: typeof tokTypes;
      tokContexts: TokContexts
    };

    jsx_readToken(): string;
    jsx_readNewLine(normalizeCRLF: boolean): void;
    jsx_readString(quote: number): void;
    jsx_readEntity(): string;
    jsx_readWord(): void;
    jsx_parseIdentifier(): acorn.Node;
    jsx_parseNamespacedName(): acorn.Node;
    jsx_parseElementName(): acorn.Node | string;
    jsx_parseAttributeValue(): acorn.Node;
    jsx_parseEmptyExpression(): acorn.Node;
    jsx_parseExpressionContainer(): acorn.Node;
    jsx_parseAttribute(): acorn.Node;
    jsx_parseOpeningElementAt(startPos: number, startLoc?: acorn.SourceLocation): acorn.Node;
    jsx_parseClosingElementAt(startPos: number, startLoc?: acorn.SourceLocation): acorn.Node;
    jsx_parseElementAt(startPos: number, startLoc?: acorn.SourceLocation): acorn.Node;
    jsx_parseText(): acorn.Node;
    jsx_parseElement(): acorn.Node;
  }
}

export = jsx;

index.d.ts Outdated
jsx_readString(quote: number): void;
}

export as namespace jsx;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This declares jsx is available as a global namespace, which is incorrect.

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
@brettz9
Copy link
Contributor Author

brettz9 commented May 13, 2022

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>

index.d.ts Outdated

declare const jsx: (options?: jsx.Options) => (BaseParser: typeof Parser) => typeof Parser;
declare const jsx: {
tokTypes: typeof acorn.tokTypes;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is written as part of the namespace, it can be imported like this when transpiling to commonjs:

import { tokTypes } from 'acorn-jsx'

Whether or not this is better, depends on the library’s intend and is subjective. I personally think that’s slightly better in this case, but I don’t have a very strong opinion on this.

Shouldn’t the type be like this?

  tokTypes: jsx.TokTypes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding use of named exports, Node.js docs state:

CommonJS modules consist of a module.exports object...

The detection of named exports is based on common syntax patterns but does not always correctly detect named exports. In these cases, using the default import form described above can be a better option.

So I'm wondering if that would be safe.

However, you are right that I should be using the jsx tokTypes instead of the Acorn one here. But it seems then that trying to fix this brings up two problems:

  1. Errors are given suggesting that the jsx in this case will be understood as the exported const rather than the namespace which we want. Renaming the namespace to jsxNS, fixes the error but then the namespace isn't exported.
  2. And because only one export is allowed at the top level, I can't apparently move interface TokTypes to the top level without being redundant (i.e., I can have one copy at the top level which is not exported and one copy within the namespace which, as you say, is auto-exported).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I solved this I think by reexporting the non-exported interface with an exported type alias. Let me know if you think this is all suitable.

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated
Comment on lines 9 to 17

type tokTypes = typeof acorn.tokTypes;

export interface TokTypes extends tokTypes {
jsxName: acorn.TokenType,
jsxText: acorn.TokenType,
jsxTagEnd: acorn.TokenType,
jsxTagStart: acorn.TokenType
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to do the following:

Suggested change
type tokTypes = typeof acorn.tokTypes;
export interface TokTypes extends tokTypes {
jsxName: acorn.TokenType,
jsxText: acorn.TokenType,
jsxTagEnd: acorn.TokenType,
jsxTagStart: acorn.TokenType
}
export interface TokTypes extends typeof acorn.tokTypes {

There’s no need to export the tokTypes type from acorn. Also it’s generally preferred to use the CamelCase for type names, which isn’t used for tokTypes. By not exporting them, this is avoided altogether.

Copy link
Contributor Author

@brettz9 brettz9 May 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I added a type alias was because I get an "expression expected" error if directly adding it as in your suggestion. See microsoft/TypeScript#14757 on why TypeScript isn't supporting this.

Should I move the type alias out of the namespace so it is not exported then?

And if so, would you prefer for CamelCase that I change it to AcornTokTypes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, and I guess this might have been inadvertent in making the commit suggestion, we don't want to delete the properties in the interface.

Copy link
Contributor Author

@brettz9 brettz9 May 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I added a type alias was because I get an "expression expected" error if directly adding it as in your suggestion. See microsoft/TypeScript#14757 on why TypeScript isn't supporting this.

Should I move the type alias out of the namespace so it is not exported then?

And if so, would you prefer for CamelCase that I change it to AcornTokTypes?

I've gone ahead and added a commit to do both of these things. Let me know if this is ok.

index.d.ts Outdated
jsxTagStart: acorn.TokenType
}

export interface Options {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Types in a namespace are always exported, so the export keyword is redundant.

It’s not wrong, but personally I prefer to remove them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Applied. TS unfortunately isn't very helpful in their docs in helping give a better understanding of ambient namespaces like this; it seems to only speak there of browser global usage. I guess this may be because modules are moving to become the preferred way, and the fact that we still need to understand the old way when in use in CJS was lost along the way.

@remcohaszing
Copy link
Contributor

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

@brettz9
Copy link
Contributor Author

brettz9 commented May 15, 2022

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

Copy link
Contributor

@remcohaszing remcohaszing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@brettz9
Copy link
Contributor Author

brettz9 commented Nov 6, 2022

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

Copy link
Member

@RReverser RReverser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and thanks for the ping - haven't looked much at Github back in May and things got lost afterwards.

@RReverser
Copy link
Member

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
Copy link
Contributor Author

brettz9 commented Nov 7, 2022

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
Copy link
Member

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
Copy link
Member

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

@RReverser RReverser changed the title feat: Support ESM declaration imports; add an AcornJsxParser class and tokTypes const Improve typings: ESM, AcornJsxParser class and tokTypes const Nov 14, 2022
@RReverser RReverser merged commit 8ed96d6 into acornjs:master Nov 14, 2022
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

Successfully merging this pull request may close these issues.

4 participants