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.

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

Shorthand ambient modules do not have a body #924

Closed
velut opened this issue Jan 28, 2021 · 2 comments · Fixed by #929
Closed

Shorthand ambient modules do not have a body #924

velut opened this issue Jan 28, 2021 · 2 comments · Fixed by #929

Comments

@velut
Copy link

velut commented Jan 28, 2021

Describe the bug

Typescript allows declaring ambient modules without a body, known as shorthand ambient modules.

// index.d.ts

declare module "foo";

Operations accessing the body of this kind of modules fail with the error Bodied node should have a body..

Version: 9.1.0
Typescript version: 4.1.3

To Reproduce

import { Node, Project, ts } from "ts-morph";

// Logs `4.1.3`
console.log(ts.version);

// Create ambient file `index.d.ts` containing a shorthand ambient module
const project = new Project();
const sourceFile = project.createSourceFile(
  "index.d.ts",
  `declare module "foo";`
);

// Module name is "foo" with quotes included
const fooModule = sourceFile.getNamespaceOrThrow('"foo"');

// Another way to access module "foo" as a simple `Node` and not a `NamespaceDeclaration`
// const fooModule = project.getAmbientModuleOrThrow('"foo"').getDeclarations()[0];

// Logs `declare module "foo";`
console.log(fooModule.getFullText());

// Throws `Bodied node should have a body.`
if (Node.isBodiedNode(fooModule)) {
  fooModule.getBody();
}

Expected behavior

Shorthand ambient modules should not be considered bodied modules.

Workaround

A possible workaround to identify shorthand ambient modules consists in checking if they have any ModuleBlock descendants.

const isShorthandAmbientModule = module.getDescendantsOfKind(SyntaxKind.ModuleBlock).length === 0
@dsherret
Copy link
Owner

dsherret commented Jan 31, 2021

Yes, this is a bug. I think probably in this case it shouldn't show up for any getNamespace methods either.

export interface NamespaceDeclaration extends ModuleDeclaration {
    readonly name: Identifier;
    readonly body: NamespaceBody;
}

export interface ModuleDeclaration extends DeclarationStatement, JSDocContainer {
    readonly kind: SyntaxKind.ModuleDeclaration;
    readonly parent: ModuleBody | SourceFile;
    readonly name: ModuleName;
    readonly body?: ModuleBody | JSDocNamespaceDeclaration;
}

Given the compiler does the above, I think probably a ModuleDeclaration node should be introduced for these.

@dsherret
Copy link
Owner

dsherret commented Feb 7, 2021

Just looking into this now... ts-morph doesn't represent ambient module declarations acurately. Also, I discovered there is an internal AmbientModuleDeclaration type in the compiler API:

export interface AmbientModuleDeclaration extends ModuleDeclaration {
    readonly body?: ModuleBlock;
}

That said, these types seem to be just for assertions.

I've been thinking about this more and ts-morph relies heavily on having a mapping from syntax kind to wrapped node. Given ModuleDeclaration, NamespaceDeclaration and AmbientModuleDeclaration all have the same kind, I'm thinking to rename NamespaceDeclaration to ModuleDeclaration and then ModuleDeclaration will handle all of these. It would have been nice if the compiler api had structured these a little nicer, but it is what it is.

I'll make these updates before the next TS major release on Feb 23rd.

dsherret added a commit that referenced this issue Feb 14, 2021
BREAKING CHANGE: ts-morph did not properly support some ambient and all shorthand ambient module declarations. Now `NamespaceDeclaration` is `ModuleDeclaration`. You will need to rename anything that references "Namespace" and change it to "Module".
@dsherret dsherret mentioned this issue Feb 18, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants