fix(ExpressionVisitor): not redeclare imports

[ben-girardet]

Fixes #537

@jdanyow could you have a look and confirm that it's OK to keep the AccessScope imported from aurelia-binding and not redeclared as any ? See my detailed question here: #537 (comment)

[ben-girardet]

@EisenbergEffect Any chance someone could review this PR ? The library is currently hard to use with Typescript 3.7+

I’m not sure I took the right approach.

[EisenbergEffect]

@bigopon You are probably the most familiar and best to review this. Do you have some time over the next week to take a look?

[bigopon]

It looks pretty safe to change, as this is pretty minimal, let's wait a bit more for @jdanyow 's response, else we should go ahead with this. It doesn't make sense to import from here anyway

[ben-girardet]

thanks guys for having a look. To help with reviewing I post here the Typescript doc related to this issue and change in Typescript 3.7:

Due to a bug, the following construct was previously allowed in TypeScript:

// ./someOtherModule.ts
interface SomeType {
    y: string;
}

// ./myModule.ts
import { SomeType } from "./someOtherModule";
export interface SomeType {
    x: number;
}

function fn(arg: SomeType) {
    console.log(arg.x); // Error! 'x' doesn't exist on 'SomeType'
}

Here, SomeType appears to originate in both the import declaration and the local interface declaration. Perhaps surprisingly, inside the module, SomeType refers exclusively to the imported definition, and the local declaration SomeType is only usable when imported from another file. This is very confusing and our review of the very small number of cases of code like this in the wild showed that developers usually thought something different was happening.

Therefore the question seems to be: "are there any known cases where other modules import AccessScope from expression-visitor ?

[jdanyow]

If I remember correctly, the export type X = any; lines were hacks to workaround some TypeScript issue or issue with the binding types. These issues probably do not exist in the latest TypeScript/binding libs.

Could you expand this change to remove all the export type X = any' lines as you've already done for AccessScope?

Thanks! 👏

[ben-girardet]

Thanks for your feedback @jdanyow

I've now done the same for the other import/export related to aurelia-binding (LiteralPrimitive and LiteralString). Now there are a few other type definition:

export type Chain = any;
export type Assign = any;
export type AccessThis = any;
export type CallScope = any;
export type CallFunction = any;
export type PrefixNot = any;
export type LiteralArray = any;
export type LiteralObject = any;

that are not exported from aurelia-binding so we cannot rely on importing them. Should we simply declare them as any in the methods declaration:

public visitChain(chain: Chain) {
  this.visitArgs(chain.expressions);
}

// becoming:

public visitChain(chain: any) {
  this.visitArgs(chain.expressions);
}

??

[jdanyow]

@ben-girardet updating the visitor's method signatures as you've described sounds good to me.

[ben-girardet]

@jdanyow @EisenbergEffect

Ready for final review and merge