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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 19 commits into from
Nov 14, 2022
Merged

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

Changes from 14 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
7b59516
feat: Support ESM declaration imports; add an `AcornJsxParser` class …
brettz9 Feb 10, 2022
dda96cc
feat: add missing `tokContexts` property on `acornJsx`
brettz9 Feb 12, 2022
baa0a61
feat: add missing jsx_ methods to `AcornJsxParser`
brettz9 Feb 12, 2022
8526524
feat: add instance properties to `AcornJsxParser`
brettz9 Feb 12, 2022
def9edd
fix: return `AcornJsxParser` from constructor
brettz9 Feb 12, 2022
2383cca
drop properties belonging on Acorn instead
brettz9 Feb 13, 2022
e4eca02
fix: use import with namespace specifier for acorn (avoids interop li…
brettz9 Apr 25, 2022
d97bd96
fix: switch `tokContexts` to type `TokContexts` (as not available as …
brettz9 May 6, 2022
620551b
refactor: avoid exporting `jsx` as named export
brettz9 May 6, 2022
835ea91
fix: in place of exporting non-existent class, export constructor and…
brettz9 May 6, 2022
5459984
refactor: use without prefix
brettz9 May 12, 2022
af6247d
refactor: export `TokTypes` interface and a single `jsx` export with …
brettz9 May 12, 2022
d80ceef
fix: drop extra `export as namespace`
brettz9 May 12, 2022
1c2fa2c
fix: use `exports = jsx` (and use jsx.Options)
brettz9 May 12, 2022
4cfaf9f
refactor: prefer interface over type, avoid redundant `export` on typ…
brettz9 May 15, 2022
41e8fcd
refactor: avoid extra type alias
brettz9 May 15, 2022
71b7e61
refactor: avoid exporting `tokTypes` and use CamelCase
brettz9 May 15, 2022
b976601
refactor: for now add a commit which redundantly but safely indicates…
brettz9 May 15, 2022
fd68170
refactor: remove some redundancy
brettz9 May 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 56 additions & 3 deletions index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,65 @@
import { Parser } from 'acorn'
import * as acorn from 'acorn';

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.

(options?: jsx.Options): (BaseParser: typeof acorn.Parser) => jsx.AcornJsxParser
}

declare namespace jsx {
interface Options {

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 •
edited
Loading

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 •
edited
Loading

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.


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.

allowNamespacedObjects?: boolean;
allowNamespaces?: boolean;
}

export type TokContexts = {
brettz9 marked this conversation as resolved.
Show resolved Hide resolved
tc_oTag: acorn.TokContext,
tc_cTag: acorn.TokContext,
tc_expr: acorn.TokContext
};

type P = typeof acorn.Parser;

// We pick (statics) from acorn rather than plain extending to avoid complaint
// about base constructors needing the same return type (i.e., we return
// `IAcornJsxParser` here)
export interface AcornJsxParser extends Pick<P, keyof P> {
brettz9 marked this conversation as resolved.
Show resolved Hide resolved
readonly acornJsx: {
tokTypes: TokTypes;
tokContexts: TokContexts
};

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

export interface IAcornJsxParser extends acorn.Parser {
brettz9 marked this conversation as resolved.
Show resolved Hide resolved
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;