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

fix: Module declaration parsed as namespace #1301

Merged
merged 5 commits into from May 15, 2020
Merged

fix: Module declaration parsed as namespace #1301

merged 5 commits into from May 15, 2020

Conversation

Unnvaldr
Copy link
Contributor

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.

@Unnvaldr Unnvaldr changed the title fix: Fix module declaration parsed as namespace fix: Module declaration parsed as namespace May 12, 2020
@Gerrit0
Copy link
Collaborator

Gerrit0 commented May 13, 2020

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).
Eventually, we'll be able to use the cleaner check that you propose, but since the former code block is still valid, we can't use these flags.


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

@Unnvaldr
Copy link
Contributor Author

The latest commit should address this "misbehaviour".
I had to use casting to any for ts.NodeFlags.Ambient because of being marked as internal and in result missing only in types declaration.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented May 14, 2020

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 ts.isStringLiteral(node.name)

@Unnvaldr
Copy link
Contributor Author

Unnvaldr commented May 14, 2020

What you posted is actually the topic of the issue, so everything is taken into the consideration. Ambient declaration is the only exception that have distinguished namespace and module and in result should have separate output. I don't think if double quotes decide about being a module or not

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 😃

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

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.

Suggested change
: createDeclaration(context, node, node.name.kind === 10
: createDeclaration(context, node, node.name.kind === ts.SyntaxKind.StringLiteral

Copy link
Collaborator

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.

@Unnvaldr Unnvaldr requested a review from Gerrit0 May 14, 2020 18:32
@Gerrit0 Gerrit0 merged commit 4fed0bd into TypeStrong:master May 15, 2020
@Gerrit0
Copy link
Collaborator

Gerrit0 commented May 15, 2020

Thanks! Should have a release out this weekend.

@Unnvaldr Unnvaldr deleted the fix/1284 branch May 15, 2020 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

declare module parsed as namespace
2 participants