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

build: typescript 3.7 support #33717

Closed
wants to merge 14 commits into from
Closed

Conversation

@andrius-pra
Copy link
Contributor

andrius-pra commented Nov 10, 2019

This PR updates TypeScript version to 3.7 while retaining compatibility with TS3.6.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #33596

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@andrius-pra andrius-pra requested review from angular/dev-infra-framework as code owners Nov 10, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Nov 10, 2019

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 @googlebot I consent. in this pull request.

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 cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no label Nov 10, 2019
@@ -150,7 +150,7 @@ describe(

it('cancel fetch should invoke onCancelTask',
ifEnvSupportsWithDone('AbortController', (done: DoneFn) => {
if (isSafari) {
if (isSafari()) {

This comment has been minimized.

Copy link
@alfaproject

alfaproject Nov 10, 2019

Contributor

<3

@@ -32,4 +32,4 @@ export const INITIAL_CONFIG = new InjectionToken<PlatformConfig>('Server.INITIAL
* @publicApi
*/
export const BEFORE_APP_SERIALIZED =
new InjectionToken<Array<() => void>>('Server.RENDER_MODULE_HOOK');
new InjectionToken<Array<() => void | Promise<void>>>('Server.RENDER_MODULE_HOOK');

This comment has been minimized.

Copy link
@andrius-pra

andrius-pra Nov 10, 2019

Author Contributor

Fixes error in this line line

asyncPromises.push(callbackResult);
                   ~~~~~~~~~~~~~~
Argument of type 'void' is not assignable to parameter of type 'Promise<any>'.
@andrius-pra

This comment has been minimized.

Copy link
Contributor Author

andrius-pra commented Nov 10, 2019

@filipesilva, I have added you as co-author because of this PR contains changes from #32962.

@@ -823,3 +821,12 @@ function getFromSymbolTable(symbolTable: ts.SymbolTable, key: string): ts.Symbol

return symbol;
}

function getTypeArguments(

This comment has been minimized.

Copy link
@andrius-pra

andrius-pra Nov 10, 2019

Author Contributor

The typeArguments property has been removed from the TypeReference interface in Typescipt 3.7. we should use the getTypeArguments function on TypeChecker instances. See #32962 (comment)

Copy link
Member

gkalpak left a comment

💯 🎉 ❤️

aio/package.json Outdated Show resolved Hide resolved
@kara kara added the comp: core label Nov 11, 2019
@ngbot ngbot bot added this to the needsTriage milestone Nov 11, 2019
@andrius-pra andrius-pra force-pushed the andrius-pra:typescript-3.7 branch from c48b942 to 8892af8 Nov 11, 2019
@andrius-pra andrius-pra force-pushed the andrius-pra:typescript-3.7 branch from 8892af8 to dc1d140 Nov 11, 2019
Copy link
Member

gkalpak left a comment

BTW, I can see several more .typeArguments accesses through-out the codebase.
Shouldn't all these occurrences be converted to use checker.getTypeArguments() if available?

@andrius-pra

This comment has been minimized.

Copy link
Contributor Author

andrius-pra commented Nov 12, 2019

.typeArguments removed only from ts.TypeReference node (microsoft/TypeScript#33693). All these occurrences are converted to use checker.getTypeArguments().

@IgorMinar IgorMinar added cla: yes and removed cla: no labels Nov 12, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Nov 12, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@IgorMinar

This comment has been minimized.

Copy link
Member

IgorMinar commented Nov 12, 2019

hi @andrius-pra, thanks for working on this PR - I just want to set the expectations right that we won't be merging this PR until we work on 9.1. I estimate that we will come back to this PR and be able to merge it in December, but unlikely sooner than that.

I'm grateful for your help, and we could use this PR in the future to get typescript 3.7 support into 9.1, but that will require cooperation with the CLI team and extensive testing that we no longer have time in 9.0 release cycle.

atscott added a commit that referenced this pull request Jan 15, 2020
atscott added a commit that referenced this pull request Jan 15, 2020
This is the tsickle version that supports TypeScript 3.7.

PR Close #33717
atscott added a commit that referenced this pull request Jan 15, 2020
With TS 3.7, these examples were running into the error below (e.g. on https://circleci.com/gh/angular/angular/574906#tests/containers/0):

```
============== AIO example output for: /home/circleci/ng/aio/content/examples/observables/
running: yarn tsc --project ./
$ /home/circleci/ng/aio/content/examples/observables/node_modules/.bin/tsc --project ./
../../../tools/examples/shared/node_modules/protractor/built/index.d.ts(5,10): error TS2440: Import declaration conflicts with local declaration of 'PluginConfig'.
../../../tools/examples/shared/node_modules/protractor/built/index.d.ts(5,24): error TS2440: Import declaration conflicts with local declaration of 'ProtractorPlugin'.
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
completed: yarn tsc --project ./
```

This happened because of angular/protractor#5348.

It's unclear why this typings problem does not affect `ng e2e` runs, and only affects `tsc` runs.

For now it seems sensible to alter the tests to compile only the app and not the e2e, since the intent of 2cc954d was never to verify the correctness of the e2e in the first place.

We still need a release of protractor that supports TS 3.7 though, but at least it doesn't seem to block our update proper.

PR Close #33717
atscott added a commit that referenced this pull request Jan 15, 2020
atscott added a commit that referenced this pull request Jan 15, 2020
atscott added a commit that referenced this pull request Jan 15, 2020
PR Close #33717
atscott added a commit that referenced this pull request Jan 15, 2020
atscott added a commit that referenced this pull request Jan 15, 2020
atscott added a commit that referenced this pull request Jan 15, 2020
Updates the commit SHA the `material-unit-tests` CircleCI
job runs against. We need to include a commit that makes
the node module installation more determinisitc (i.e. ensuring
that `tsickle` is always hoisted at the node module root).

angular/components@31a50f7

PR Close #33717
atscott added a commit that referenced this pull request Jan 15, 2020
The components repository has a Yarn resolution to ensure that
dgeni-packages uses a specific TypeScript version. This resolution
causes the specified TS version to be considered as candidate for the
`@angular/bazel` peer dependency. Ultimately, Yarn decides to use
the TypeScript version from the resolution for `@angular/bazel`,
and builds will fail due to a version mismatch.

This is because `tsickle` will use the hoisted top-level TS
version (set to `3.7.4` ), while `@angular/bazel` uses the
version from the resolution (at the time of writing: v3.6.4)

PR Close #33717
atscott added a commit that referenced this pull request Jan 15, 2020
Updates the SHA that will be tested against in the `material-unit-tests` job to the latest commit in the components repository. SHA 71955d2e194bfc5561f25daea16e68af266d6ff9 is needed in order to compile repository with typescript 3.7

PR Close #33717
atscott added a commit that referenced this pull request Jan 15, 2020
Just to be consistent.

PR Close #33717
atscott added a commit that referenced this pull request Jan 15, 2020
This PR updates TypeScript version to 3.7 while retaining compatibility with TS3.6.

PR Close #33717
atscott added a commit that referenced this pull request Jan 15, 2020
atscott added a commit that referenced this pull request Jan 15, 2020
This is the tsickle version that supports TypeScript 3.7.

PR Close #33717
atscott added a commit that referenced this pull request Jan 15, 2020
With TS 3.7, these examples were running into the error below (e.g. on https://circleci.com/gh/angular/angular/574906#tests/containers/0):

```
============== AIO example output for: /home/circleci/ng/aio/content/examples/observables/
running: yarn tsc --project ./
$ /home/circleci/ng/aio/content/examples/observables/node_modules/.bin/tsc --project ./
../../../tools/examples/shared/node_modules/protractor/built/index.d.ts(5,10): error TS2440: Import declaration conflicts with local declaration of 'PluginConfig'.
../../../tools/examples/shared/node_modules/protractor/built/index.d.ts(5,24): error TS2440: Import declaration conflicts with local declaration of 'ProtractorPlugin'.
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
completed: yarn tsc --project ./
```

This happened because of angular/protractor#5348.

It's unclear why this typings problem does not affect `ng e2e` runs, and only affects `tsc` runs.

For now it seems sensible to alter the tests to compile only the app and not the e2e, since the intent of 2cc954d was never to verify the correctness of the e2e in the first place.

We still need a release of protractor that supports TS 3.7 though, but at least it doesn't seem to block our update proper.

PR Close #33717
atscott added a commit that referenced this pull request Jan 15, 2020
atscott added a commit that referenced this pull request Jan 15, 2020
atscott added a commit that referenced this pull request Jan 15, 2020
atscott added a commit that referenced this pull request Jan 15, 2020
atscott added a commit that referenced this pull request Jan 15, 2020
atscott added a commit that referenced this pull request Jan 15, 2020
Updates the commit SHA the `material-unit-tests` CircleCI
job runs against. We need to include a commit that makes
the node module installation more determinisitc (i.e. ensuring
that `tsickle` is always hoisted at the node module root).

angular/components@31a50f7

PR Close #33717
atscott added a commit that referenced this pull request Jan 15, 2020
The components repository has a Yarn resolution to ensure that
dgeni-packages uses a specific TypeScript version. This resolution
causes the specified TS version to be considered as candidate for the
`@angular/bazel` peer dependency. Ultimately, Yarn decides to use
the TypeScript version from the resolution for `@angular/bazel`,
and builds will fail due to a version mismatch.

This is because `tsickle` will use the hoisted top-level TS
version (set to `3.7.4` ), while `@angular/bazel` uses the
version from the resolution (at the time of writing: v3.6.4)

PR Close #33717
atscott added a commit that referenced this pull request Jan 15, 2020
Updates the SHA that will be tested against in the `material-unit-tests` job to the latest commit in the components repository. SHA 71955d2e194bfc5561f25daea16e68af266d6ff9 is needed in order to compile repository with typescript 3.7

PR Close #33717
atscott added a commit that referenced this pull request Jan 15, 2020
Just to be consistent.

PR Close #33717
kyliau added a commit to kyliau/vscode-ng-language-service that referenced this pull request Jan 15, 2020
angular/angular#33717 upgraded Angular monorepo
to TypeScript 3.7.4, so we could now upgrade to ts 3.7

This would fix the error with "no config file found for *.html" if no
TypeScript file is open.

PR closes angular#546
ayazhafiz added a commit to angular/vscode-ng-language-service that referenced this pull request Jan 15, 2020
angular/angular#33717 upgraded Angular monorepo
to TypeScript 3.7.4, so we could now upgrade to ts 3.7

This would fix the error with "no config file found for *.html" if no
TypeScript file is open.

PR closes #546
@@ -228,19 +229,19 @@ export declare class NgForOf<T, U extends NgIterable<T> = NgIterable<T>> impleme
export declare class NgForOfContext<T, U extends NgIterable<T> = NgIterable<T>> {
$implicit: T;
count: number;
readonly even: boolean;
readonly first: boolean;
get even(): boolean;

This comment has been minimized.

Copy link
@StefanKern

StefanKern Jan 16, 2020

Why did you switch from readonly to get?

  • curious angular developer

This comment has been minimized.

Copy link
@filipesilva

filipesilva Jan 16, 2020

Member

We didn't, at least intentionally.

These .d.ts files in tools/public_api_guard/common are snapshots of the types for the Angular API created by ts-api-guardian. The purpose of that tools is to tell us when the public TS api changed. This is useful to detect cases where we did some code change that accidentally changed the API, in which case we revert the change.

In this case however, TS started emitting different types so we had to update the snapshots. TS does not follow semver and can be expected to emit different type output between minors, so it is ok to update the snapshots.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.