JamesHenry / typescript-estree

:sparkles: A parser that converts TypeScript source code into an ESTree-compatible form
https://jameshenry.blog
Other
84 stars 13 forks source link

feat: align TSModuleDeclaration with babel-typescript #38

Closed armano2 closed 5 years ago

armano2 commented 5 years ago

This PR adds optional global property to TSModuleDeclaration AST in case when

// Augmentations for the global stuff.
declare global {
}

In Babel/Typescript we have

defineType("TSModuleDeclaration", {
  aliases: ["Statement", "Declaration"],
  visitor: ["id", "body"],
  fields: {
    declare: validateOptional(bool),
    global: validateOptional(bool),
    id: validateType(["Identifier", "StringLiteral"]),
    body: validateType(["TSModuleBlock", "TSModuleDeclaration"]),
  },
});

fixes: #27, https://github.com/eslint/typescript-eslint-parser/issues/570

JamesHenry commented 5 years ago

Nice one, @armano2, thanks!

armano2 commented 5 years ago

thank you đź‘Ť

mysticatea commented 5 years ago

Hmm, global augmentation is not a module declaration, so I feel wrongness if we express it as ModuleDeclaration node. May it be fine to follow babel...?

(declare global is neither declare module nor declare namespace)

armano2 commented 5 years ago

@mysticatea typescript is using one node for it to, with node flags

https://www.typescriptlang.org/docs/handbook/namespaces-and-modules.html

A note about terminology: It’s important to note that in TypeScript 1.5, the nomenclature has changed. “Internal modules” are now “namespaces”. “External modules” are now simply “modules”, as to align with ECMAScript 2015’s terminology, (namely that module X { is equivalent to the now-preferred namespace X {)

mysticatea commented 5 years ago

declare global {} is neither module X {} nor namespace X {}.

JamesHenry commented 5 years ago

:tada: This PR is included in version 5.3.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

armano2 commented 5 years ago

declare states for augmentation https://www.typescriptlang.org/docs/handbook/declaration-merging.html#global-augmentation

declare global {} is augmentation of global namespace declare module 'name' is augmentation of named module


btw. i'm not disagreeing with you, and i think its weird to have one node for this.

https://github.com/JamesHenry/typescript-estree#ast-alignment-tests

mysticatea commented 5 years ago

It's the related argument in https://github.com/JamesHenry/typescript-estree/issues/28. Currently we have inconsistent expressions for the declare...

mysticatea commented 5 years ago

In fact, JavaScript AST and TypeScript AST have really different mental models. The fact has appeared in, for example, https://github.com/tc39/proposal-decorators/issues/69.

In JavaScript AST, it expresses semantics as independent nodes. E.g., ExportDeclaration nodes export declarations that the node has. In this parspective, independent nodes for each semantic is natural. I think that global argmentation is a kind of nodes and ambient declaration is also a kind of nodes.

On the other hand, TypeScript AST closes to the source code. In TypeScript AST, export is an attribute of declarations. Also declare is an attribute of declarations. Those are the element of source code rather than semantics.

ESTree is the former, but TypeScript is the latter. In other word, in typescript-estree, the JS part is the former and the TS part is the latter. I feel that the mix causes confusion, but I'm not sure how we should do.

armano2 commented 5 years ago

hmm, than maybe we should go for wrapper node as you proposed in https://github.com/JamesHenry/typescript-estree/issues/28


this PR got merged really fast xd