-
Notifications
You must be signed in to change notification settings - Fork 25.3k
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
Ts 3.7-beta support #32962
Ts 3.7-beta support #32962
Conversation
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
You can preview ee6157b at https://pr32962-ee6157b.ngbuilds.io/. |
microsoft/TypeScript#33693 is a breaking change that affects us. It's easy to work around though: before:
after
|
Hi @filipesilva !
At, for exampe,
Obviously I know I should not use 3.7, it's just "for fun". |
You can preview 5838059 at https://pr32962-5838059.ngbuilds.io/. |
Heya @lppedd, thanks for letting us know. I think the integration tests in this repo should catch the same error. |
You can preview 4ac0c72 at https://pr32962-4ac0c72.ngbuilds.io/. |
You can preview a0c0e20 at https://pr32962-a0c0e20.ngbuilds.io/. |
@@ -195,8 +195,9 @@ class TypeScriptSymbolQuery implements SymbolQuery { | |||
const type = this.checker.getTypeAtLocation(parameter.type !); | |||
if (type.symbol !.name == 'TemplateRef' && isReferenceType(type)) { | |||
const typeReference = type as ts.TypeReference; | |||
if (typeReference.typeArguments && typeReference.typeArguments.length === 1) { | |||
return typeReference.typeArguments[0].symbol; | |||
const typeArguments = this.checker.getTypeArguments(typeReference); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #32962 (comment)
@@ -9,7 +9,7 @@ | |||
import {Type} from '../interface/type'; | |||
import {TypeDecorator, makeDecorator} from '../util/decorators'; | |||
|
|||
import {InjectableType, getInjectableDef, ɵɵInjectableDef, ɵɵdefineInjectable} from './interface/defs'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix for
packages/core/src/di/injectable.ts(12,9): error TS2440: Import declaration conflicts with local declaration of 'InjectableType'.
Listed under breaking changes in https://devblogs.microsoft.com/typescript/announcing-typescript-3-7-beta/.
@@ -1269,7 +1269,7 @@ function saveNameToExportMap( | |||
exportsMap[def.exportAs[i]] = index; | |||
} | |||
} | |||
if ((def as ComponentDef<any>).template) exportsMap[''] = index; | |||
exportsMap[''] = index; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix for
packages/core/src/render3/instructions/shared.ts(1272,9): error TS2774: This condition will always return true since the function is always defined. Did
you mean to call it instead?
Listed under breaking changes in https://devblogs.microsoft.com/typescript/announcing-typescript-3-7-beta/.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The if
guard is there to differentiate between DirectiveDef
and ComponentDef
, by testing to see if def
has a template
property. This is probably still relevant, so a better fix might be
exportsMap[''] = index; | |
if ((def as ComponentDef<any>).template !== undefined) exportsMap[''] = index; |
or
exportsMap[''] = index; | |
if (!!(def as ComponentDef<any>).template) exportsMap[''] = index; |
(which is the alternative workaround in the notes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cast to ComponentDef<any>
looks odd to me since ComponentDef
always has a template. It's means "say that def is a ComponentDef, is the template property defined?" to which it makes sense that TS would complain.
Shouldn't the cast be something different then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that pattern is a bit misleading. In this case, the cast is there as an assumption of it indeed being a ComponentDef
, then checking if template
actually exists to confirm.
This pattern is often used in functions with type predicates:
export function isComponentDef<T>(def: DirectiveDef<T>|ComponentDef<T>): def is ComponentDef<T> {
return (def as ComponentDef<T>).template !== undefined;
}
In this case however, the compiler requires the return type to be boolean, so the original guard as written before you changed it would not have compiled in the type guard. I guess the extra function is omitted for code size (although I'm not sure on that one).
The benefit of the "assumed cast" is that there's proper reference tracking, i.e. IDEs know that the template
property read does in fact correspond with ComponentDef.template
, so e.g. renaming template
will correctly update the isComponentDef
function.
Compilation currently fails with this error:
This corresponds to the following code in
This seems to be another instance of #32962 (comment). Reported as angular/tsickle#1096 |
After updating to CLI 8.3.9 I started receiving the following error on build (both dev and prod).
Latest 3.7-dev. |
@lppedd the I'm not sure what that last error is. Was that on |
@filipesilva thanks! And yes, that last |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
You can preview 42aef61 at https://pr32962-42aef61.ngbuilds.io/. |
This seems to have been superceded by #33717. Can we close this, @filipesilva? |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Just testing to see what breaks if #32946 is followed up by a 3.7-beta update.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information