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(ExpressionVisitor): not redeclare imports #538

Merged
merged 1 commit into from Dec 8, 2019

Conversation

@ben-girardet
Copy link
Contributor

ben-girardet commented Dec 3, 2019

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

This comment has been minimized.

Copy link
Contributor Author

ben-girardet commented Dec 6, 2019

@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

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Dec 6, 2019

@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

This comment has been minimized.

Copy link
Member

bigopon commented Dec 6, 2019

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

This comment has been minimized.

Copy link
Contributor Author

ben-girardet commented Dec 6, 2019

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 ?

Copy link
Member

jdanyow left a comment

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

This comment has been minimized.

Copy link
Contributor Author

ben-girardet commented Dec 6, 2019

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

This comment has been minimized.

Copy link
Member

jdanyow commented Dec 7, 2019

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

@ben-girardet ben-girardet force-pushed the ben-girardet:fix-typing branch from 67ef9ea to 5cd8d70 Dec 8, 2019
@ben-girardet ben-girardet changed the title fix(ExpressionVisitor): not redeclare AccessScope fix(ExpressionVisitor): not redeclare imports Dec 8, 2019
@ben-girardet

This comment has been minimized.

Copy link
Contributor Author

ben-girardet commented Dec 8, 2019

@jdanyow @EisenbergEffect

Ready for final review and merge

@jdanyow
jdanyow approved these changes Dec 8, 2019
@jdanyow jdanyow merged commit 15109b3 into aurelia:master Dec 8, 2019
4 checks passed
4 checks passed
WIP Ready for review
Details
ci/circleci: build_test Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details
main Workflow: main
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.