Breaking: Add .body to TSModuleBlock nodes (fixes #217)

[flying-sheep]

I’ll probably have to fix some tests and don’t know if just choosing the name “ModuleBlock” is OK.

[soda0289]

Since the node is specific to typescript we should prefix it with TS. TSModuleBlock This is also a breaking change I think the commit message has to prefixed with Breaking:

[eslintbot]

Thanks for the pull request, @flying-sheep! I took a look to make sure it's ready for merging and found some changes are needed:

• The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

[soda0289]

@Pajn Will this change mess up the prettier work that has been completed?

[eslintbot]

LGTM

[flying-sheep]

OK, all done

[eslintbot]

LGTM

[Pajn]

@Pajn Will this change mess up the prettier work that has been completed?

No, modules are not started yet so it's fine. Also more similarity with ESTree is nicer there as well so 👍

[soda0289]

@flying-sheep Looks like a namespace can also have import statements. We should update those as well.

declare namespace 'Baz' {
  import {Bar} from 'bar';
}

[flying-sheep]

namespaces already convert to TSModuleDeclaration → TSModuleBlock.

please check the modified test file (declare-namespace-with-exported-function.result.js), which is the result of converting a namespace.

[soda0289]

@flying-sheep Sorry missed that.

Is there a test for declared modules? I don't see any tests that failed. We should have one for declared modules to ensure we don't break anything in the future.

[eslintbot]

LGTM

[flying-sheep]

there’s one for shorthand declared modules (without body), so i added a full one. with import statement for good measure

[soda0289]

Thanks! LGTM

[JamesHenry]

LGTM, thanks!

[JamesHenry]

Seems like we have a couple of breaking changes we can look to merge now, so we should get them in together

[JamesHenry]

And another #233 😄

[azz]

I'm pretty sure SyntaxKind.ModuleBlock is legacy in name. The namespace keyword was originally module. They're now aliases, but namespace is recommended.

Since we have more freedom here, should we call the node TSNamespaceBlock instead?

[JamesHenry]

@azz module and namespace are not always aliases

Being able to declare an ambient module name for the tsc is a useful feature for providing type information for 3rd party code. Namespace, however, cannot be used for this functionality.

E.g. the difference here:
https://www.typescriptlang.org/play/#src=declare%20module%20'lodash'%20%7B%20%7D%0D%0A%0D%0Adeclare%20namespace%20'lodash'%20%7B%7D

And AST confirmation (see the parseDiagnostics - those are parse errors):
http://astexplorer.net/#/gist/d63a43d869829f9a267517f036b4a22a/720b043dfff188b0aa1cadc1f55662a434efdb20

Reference for shorthand ambient module declarations: https://github.com/Microsoft/TypeScript/wiki/What's-new-in-TypeScript#shorthand-ambient-module-declarations

[azz]

@JamesHenry I was aware of the difference, but I was not aware that declare module "x" {} used the same AST nodes (ModuleDeclaration & ModuleBlock). Thanks.