-
-
Notifications
You must be signed in to change notification settings - Fork 727
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
fix: Module declaration parsed as namespace #1301
Conversation
Thanks for the PR! It looks like there's a problem with this. module Foo { } and namespace Foo { } Should result in the same output (both are namespaces according to TD). The TS docs have apparently removed any useful reference to this legacy syntax. It's still on archive.org though. https://web.archive.org/web/20190317062641/https://www.typescriptlang.org/docs/handbook/namespaces-and-modules.html |
The latest commit should address this "misbehaviour". |
I'm afraid that doesn't work either. Ambient doesn't mean module. declare module Foo {} // Has Ambient flag
declare namespace Foo2 {} // Has ambient flag I think the best we can do for now is check |
What you posted is actually the topic of the issue, so everything is taken into the consideration. edit: Indeed, the difference is here, so I've changed the method. Though I didn't use your function, but actually a simple kind check, so if this isn't ok feel free to manually alter it. Thanks for pointing out the difference 😃 |
src/lib/converter/nodes/module.ts
Outdated
@@ -24,7 +24,9 @@ export class ModuleConverter extends ConverterNodeComponent<ts.ModuleDeclaration | |||
convert(context: Context, node: ts.ModuleDeclaration): Reflection | undefined { | |||
const reflection = context.isInherit && context.inheritParent === node | |||
? <DeclarationReflection> context.scope | |||
: createDeclaration(context, node, ReflectionKind.Namespace); | |||
: createDeclaration(context, node, node.name.kind === 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the kind is fine (that's all the function does) but the inline constant could break across different TS versions. We should instead use the enum.
: createDeclaration(context, node, node.name.kind === 10 | |
: createDeclaration(context, node, node.name.kind === ts.SyntaxKind.StringLiteral |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have time later today or tomorrow to pull this down and fix if you don't have time.
Thanks! Should have a release out this weekend. |
Fixes #1284
I stumbled upon this particular issue and noticed that module declaration lacks
ts.NodeFlags.Namespace
in AST, so it was very easy to "fix". I've checked if it works, but it may need further investigation.