Fix: Label variable declartions as Ambient (fixes #185)

[Pajn]

Sets a new type on ambient variables as discussed in #185. I tried to follow the implementation in #163

[eslintbot]

LGTM

[soda0289]

I think this should be TSAmbientVariableDeclaration since it fits better with estree. What was the reason behind VariableDefinition?

[Pajn]

You used definition for the the methods in #163 and I just followed that because that what I based this PR on. But I agree with you Declaration would probably be better in this case, will update.

[soda0289]

Thanks I just feel like we should be as close to estree as possible.

[eslintbot]

LGTM

[soda0289]

LGTM

[JamesHenry]

I think the downside of this is that the standard ESLint rules which apply to VariableDeclarations will stop working as expected e.g. no-semi.

That tradeoff is probably worth it for now...

[Pajn]

I could add a property to the node instead, maybe "declare": true similarly to how Babylon 7 recently changed from "ForAwaitStatement" to "ForOfStatement" prettier/prettier#1073

I have no idea what approach is the best in this case though.

[soda0289]

@JamesHenry @Pajn
I think whatever method we choose it should be consistent for all ambient declarations.

The reason I recommended using a new node type was because I found that empty body Function Expressions/Declarations, those used in declared functions or ambient class declarations, caused rules to break in eslint. I know we have renamed node types before, such as Abstract Classes or Functions in Namespaces, to overcome rule failures and thought that solution might work here as well.

If we didn't use new node types we can always fix the rules in eslint, and other plugins. So I would not be opposed to using a property.

Another thing I just though of is that using the method ts.isInAmbientContext(node) might now work here since it is true not only when the declare keyword is used but I also think it is true when a variable is in an ambient namespace or module.

declare namespace Foo {
   const bar = 0; //I think isInAmbientContext is true
}

Will that be a problem? Should we use two different properties for declared or isAmbient?

[Pajn]

I think that when changing the structure of the nodes (like not having a body in a function), the type should change as those probably will be handled differently both by eslint rules and by pretter.

Will that be a problem? Should we use two different properties for declared or isAmbient?

Uh, yes. At least for pretter it's important to differentiate them as it needs to know if it should print "declare " or not. It might be less important for lint rules as those probably will apply the same rules in either case, but I think they should have the ability to differentiate them because you never know what rules that may come up in the future.
While it is possible to figure it out by looking at the parents I don't think you should have to. Each node should contain enough information to be able to describe itself.
Good thing that you thought of that! 👍

[JamesHenry]

I am leaning towards a declared boolean property the more I think about this...

So many standard constructs in TypeScript can also be "declared" to turn them into ambient type information, rather than true implementation details.

declare var foo
declare function bar() {}
declare class Qux {}
etc

By simply marking the nodes as declared: true, it would allow us to keep using standard ESTree nodes wherever possible (rather than an endless list of TSNodes), make it easier for downstream tools like prettier to get everything they need from the AST, and allow standard ESLint rules (such as no-semi) to continue to work.

@Pajn please could you add @soda0289's example case above as a test either way?

We will still need to separately address the point around empty bodies on ESTree nodes (which other tools are expecting to be there), but can you guys think of any other downsides of going with the declared: Boolean approach for this?

[soda0289]

@JamesHenry
I think it makes sense to use a property declared. This will break some eslint rules since declared classes can have empty body functions. But I'm leaning towards fixing those rules instead of making custom nodes in this parser.

We could even change abstract methods and classes to just use a property as well.

[soda0289]

Could you change this to use a property declared?

[JamesHenry]

Thanks a lot for your contributions to the project, @Pajn!

I am going to close this now, as significant changes have been made to the codebase since this was first opened.

We are tracking similar changes to this one here: https://github.com/JamesHenry/tsep-babylon-test/issues/6

[Pajn]

I'm sorry for having letting it drag out. Your collaboration with the TS team is super exciting, great work!