Skip to content

Fix noResolution for named imports#636

Merged
Perryvw merged 4 commits intoTypeScriptToLua:masterfrom
hazzard993:no-resolution-fix
Jun 20, 2019
Merged

Fix noResolution for named imports#636
Perryvw merged 4 commits intoTypeScriptToLua:masterfrom
hazzard993:no-resolution-fix

Conversation

@hazzard993
Copy link
Copy Markdown
Contributor

@hazzard993 hazzard993 commented Jun 19, 2019

Closes #635

@noResolution should still only be applicable to ambient modules.

If a module is merged with another and one has a @noResolution, no path resolution will be performed.

/** @noResolution */
declare module "module" {}
declare module "module" {
    export const x: number;
}
// ✔ will stay "module"
import { x } from "module";

ark120202
ark120202 reviewed Jun 19, 2019
View reviewed changes
src/LuaTransformer.ts Outdated
const shouldResolve = !tsHelper.getCustomDecorators(type, this.checker).has(DecoratorKind.NoResolution);
let shouldResolve = true;
if (ts.isNamedImports(imports)) {
for (const importSpecifier of imports.elements) {
Copy link
Copy Markdown
Contributor

@ark120202 ark120202 Jun 19, 2019

Choose a reason for hiding this comment

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

I don't think we can rely on elements there. First, what about import {} from "fake"? Second, I'm not sure if that would work with re-exporting.
We also have to consider a case when we have no elements to check: import "fake".
I think we should check out the actual type of module, not types of imported elements. I'm not really sure how to do it, but editors that use TSLS seem to have resolved module path on hover, so type checker should have it

Copy link
Copy Markdown
Contributor Author

@hazzard993 hazzard993 Jun 19, 2019

Choose a reason for hiding this comment

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

Thanks for the hand! I can't think of a way to test import {} from "fake" but I've got import "fake" validated by two new tests.

Also using this.checker.getSymbolAtLocation(statement.moduleSpecifier) looks much more reliable

@ark120202
Copy link
Copy Markdown
Contributor

ark120202 commented Jun 19, 2019

Looks like we can use this.checker.getSymbolAtLocation(statement.moduleSpecifier) to get it in all cases. It's a symbol, so instead of getCustomDecorators it should use collectCustomDecorators to get decorators.

@hazzard993
Copy link
Copy Markdown
Contributor Author

hazzard993 commented Jun 19, 2019

I've removed the isNonNamespaceModuleDeclaration method as the more I think about it the more useless it seems to be.

This directive can technically be used on a namespace and a module. General rule is if an import statement is importing something that comes from a namespace or module with the @noResolution directive, no resolution attempts will be made.

Perryvw
Perryvw reviewed Jun 19, 2019
View reviewed changes
src/LuaTransformer.ts Outdated
let shouldResolve = true;
const moduleOwnerSymbol = this.checker.getSymbolAtLocation(statement.moduleSpecifier);
if (moduleOwnerSymbol) {
const decMap = new Map<DecoratorKind, Decorator>();
Copy link
Copy Markdown
Member

@Perryvw Perryvw Jun 19, 2019

Choose a reason for hiding this comment

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

Don't abbreviate variable names please.

@Perryvw Perryvw merged commit bccecc5 into TypeScriptToLua:master Jun 20, 2019
@hazzard993 hazzard993 deleted the no-resolution-fix branch June 20, 2019 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Perryvw Perryvw Perryvw left review comments

+1 more reviewer

@ark120202 ark120202 ark120202 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

noResolution directive not detected on named imports

3 participants

@hazzard993 @ark120202 @Perryvw