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
feat: add support for TypeScript 3.1 #26151
Conversation
You can preview 4f566d8 at https://pr26151-4f566d8.ngbuilds.io/. |
You can preview 6bd9e22 at https://pr26151-6bd9e22.ngbuilds.io/. |
We no longer support these versions and the tests actually break with the output from 3.1 (at least in the case of tsc 2.9)
tap results look good - only preexisting failures |
You can preview 51ead0e at https://pr26151-51ead0e.ngbuilds.io/. |
this.patchedDeps && ngBackPatch_node_modules_libB_module() && (this.patchedDeps = true); | ||
return details_elided;} | ||
patchedDeps: false, create(parentInjector: Injector | null): NgModuleRef<AppModule>{ | ||
if (!this.patchedDeps) { |
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.
@mhevery is this the right change?
The original code is wrong and doesn't compile, when you read it you realize that it results in unreachable code because ngBackPatch_node_modules_libB_module() doesn't explicitly return anything (so it returns void).
The spec suggests that this is our generated code. Does that mean that the compiler actually emits bad code?
Please advise on how to proceed with this change.
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.
This whole file is outdated and should be deleted. These tests don't even have a runner.
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 .d.ts
files look strange to me because they have value rather than a type. LGTM otherwise.
@@ -1497,8 +1497,8 @@ export interface HttpDownloadProgressEvent extends HttpProgressEvent { | |||
export declare class HttpErrorResponse extends HttpResponseBase implements Error { | |||
readonly error: any | null; | |||
readonly message: string; | |||
readonly name: string; | |||
readonly ok: boolean; | |||
readonly name = "HttpErrorResponse"; |
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.
This seems wrong. Why does the .d.ts
file have the actual value rather than the type. Can you look into it?
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.
I believe that ts 3.1 now stores literal values used in prop initializers directly in d.ts. Previously only the inferred type would be present in d.ts
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. confirmed.
@@ -624,7 +624,7 @@ export declare abstract class Query { | |||
|
|||
export declare class QueryList<T> { | |||
readonly changes: Observable<any>; | |||
readonly dirty: boolean; | |||
readonly dirty = true; |
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.
same in this file
We no longer support these versions and the tests actually break with the output from 3.1 (at least in the case of tsc 2.9) PR Close #26151
The CLI legacy e2e tests where failing because Angular 7.0.0-rc.0 embed angular/angular#26151, and TS version 3.0 wasn't right for this. This commit fix this error as reported in angular#12423, but isn't enought to support TS 3.1. Close angular#12413
We no longer support these versions and the tests actually break with the output from 3.1 (at least in the case of tsc 2.9) PR Close angular#26151
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. |
No description provided.