Skip to content

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 😃

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.

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