Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

no-duplicate-imports: Allow duplicate imports from separate ambient module declarations #3398

Merged
merged 2 commits into from Oct 26, 2017
Merged

Conversation

ghost
Copy link

@ghost ghost commented Oct 26, 2017

PR checklist

Overview of change:

Allows two ambient modules to have separate imports:

declare module "a" {
    import foo from "foo";
}
declare module "b" {
    import foo from "foo";
}

Preserves existing behavior for module augmentations.

@ajafff It would be nice to reuse more code from findImports in tsutils but it doesn't seem possible given that it currently takes the whole source file as input, so I can't run it on a single declare module.

CHANGELOG.md entry:

[bugfix] no-duplicate-imports: Allow duplicate imports from separate ambient module declarations

@adidahiya adidahiya requested a review from ajafff October 26, 2017 17:53
Copy link
Contributor

@ajafff ajafff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a suggestion

seen.add(text);
}

if (isModuleDeclaration(statement) && statement.body !== undefined && isModuleBlock(statement.body)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could add an additional check here if statement.name.kind === ts.SyntaxKind.StringLiteral, because namespaces cannot contain import statements.
and then you probably don't need the isModuleBlock check.

case ts.SyntaxKind.ImportDeclaration:
return (statement as ts.ImportDeclaration).moduleSpecifier;
case ts.SyntaxKind.ImportEqualsDeclaration:
const ref = (statement as ts.ImportEqualsDeclaration).moduleReference;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It didn't check ImportEqualsDeclaration before your change. That's not a problem, but should be mentioned in the changelog.

Copy link
Author

@ghost ghost Oct 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was right before because you might want named imports from a module in addition to importing it as a function. Removed this part.

@ajafff ajafff merged commit 0176122 into palantir:master Oct 26, 2017
@ajafff
Copy link
Contributor

ajafff commented Oct 26, 2017

thanks @andy-ms

@ghost ghost deleted the no-duplicate-imports branch October 26, 2017 20:10
@mxl
Copy link
Contributor

mxl commented Nov 13, 2017

@ajafff Could you make a release with these changes?

@ajafff
Copy link
Contributor

ajafff commented Nov 13, 2017

@mxl I could prepare a release PR, but I cannot do the actual release.
In addition there are other PRs that should be included in the next release. Seems like we just need to wait 😕

HyphnKnight pushed a commit to HyphnKnight/tslint that referenced this pull request Apr 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants