Skip to content
This repository was archived by the owner on Jan 14, 2019. It is now read-only.

feat: align TSModuleDeclaration with babel-typescript #38

Merged
merged 1 commit into from
Nov 30, 2018

Conversation

armano2
Copy link
Contributor

@armano2 armano2 commented Nov 30, 2018

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, eslint/typescript-eslint-parser#570

@armano2 armano2 force-pushed the global-module-declaration branch from dcd8e88 to f2015fa Compare November 30, 2018 00:49
@JamesHenry
Copy link
Owner

Nice one, @armano2, thanks!

@JamesHenry JamesHenry merged commit 13e2a24 into JamesHenry:master Nov 30, 2018
@armano2 armano2 deleted the global-module-declaration branch November 30, 2018 01:00
@armano2
Copy link
Contributor Author

armano2 commented Nov 30, 2018

thank you 👍

@mysticatea
Copy link

mysticatea commented Nov 30, 2018

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)

JamesHenry added a commit that referenced this pull request Nov 30, 2018
@armano2
Copy link
Contributor Author

armano2 commented Nov 30, 2018

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

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

@JamesHenry
Copy link
Owner

🎉 This PR is included in version 5.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@armano2
Copy link
Contributor Author

armano2 commented Nov 30, 2018

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

It's the related argument in #28. Currently we have inconsistent expressions for the declare...

@mysticatea
Copy link

In fact, JavaScript AST and TypeScript AST have really different mental models. The fact has appeared in, for example, tc39/proposal-decorators#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 armano2 changed the title feat: align TSModuleDeclaration with babel/eslint feat: align TSModuleDeclaration with babel-typescript Nov 30, 2018
@armano2
Copy link
Contributor Author

armano2 commented Nov 30, 2018

hmm, than maybe we should go for wrapper node as you proposed in #28


this PR got merged really fast xd

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

Successfully merging this pull request may close these issues.

cannot distinguish declare global on AST.
3 participants