refactor(cdk/drag-drop): Correctly type drag handle parent drag witho…

[Achilles1515]

…ut circular dependencies warning

Import the CdkDrag directive reference as a type-only import to avoid the circular dependency warning.

[crisbeto]

The change seems fine, but as evidenced by the CI, it still introduces a circular import. I believe the check works by looking at static dependencies between files and it doesn't distinguish between type-only and hard dependencies.

[Achilles1515]

The change seems fine, but as evidenced by the CI, it still introduces a circular import. I believe the check works by looking at static dependencies between files and it doesn't distinguish between type-only and hard dependencies.

Oh, I assumed the circular dependency comments were referring to outputs from the Angular CLI (or TypeScript checker or whatever is running behind the scenes) - adding the type-only import on my project no longer threw the circular dependency warning. So are you guys running an entirely different set of checks?
Maybe my project setup isn't correct.

Regarding the other errors:
Type 'undefined' is not assignable to type 'CdkDrag<any> | null'

When an @Optional() is not found, Angular assigns the value of null, yet there are multiple places where the optional parameters syntax, ?: (i.e. undefined), is being used...for correctness, should all these usages be changed to <value> | null?

[crisbeto]

I think the check comes from @angular/dev-infra-private and is based on static analysis of the source files, rather than the transpiled JS. You can run it against this repo using yarn ts-circular-deps:check.

As for the @Optional type, we've looked into marking all of them as | null in the past, but the problem is that ViewEngine will throw an error because of it. Turning them into optional parameters (:?) does work, but we'd have to create a bunch of breaking constructor changes so we decided to do it once ViewEngine has been removed.

[Achilles1515]

I think the check comes from @angular/dev-infra-private and is based on static analysis of the source files, rather than the transpiled JS. You can run it against this repo using yarn ts-circular-deps:check.

Yeah, that looks right.

Perhaps some type of check for type-only imports needs to be added here:

https://github.com/angular/dev-infra-private-builds/blob/728d773801f40fb11bf8ec29032c8a7067caf142/ts-circular-dependencies/parser.js#L26-L37

A nice isTypeOnlyImportOrExportDeclaration(node: Node) utility function exists:

https://github.com/microsoft/TypeScript/blob/master/src/compiler/utilitiesPublic.ts#L1102

So this probably garners opening a new issue for supporting type-only imports, in a general sense, in this script. Is this something you want to do as that is a repo of "private" tools or is there another repo where this issue should be opened?

As for the @Optional type, we've looked into marking all of them as | null in the past, but the problem is that ViewEngine will throw an error because of it. Turning them into optional parameters (:?) does work, but we'd have to create a bunch of breaking constructor changes so we decided to do it once ViewEngine has been removed.

Good to know. I changed it back to undefined instead of null in this PR.

[andrewseguin]

@Achilles1515 Sorry for the lack of attention on this - do you mind rebasing? @crisbeto Does this change look okay to merge?

[Achilles1515]

New pull request for this issue:
#24664

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}