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: update to TypeScript 3.4 #29372

Closed
wants to merge 1 commit into from

Conversation

@filipesilva
Copy link
Member

commented Mar 18, 2019

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 new behavior?

Update to TS 3.4: https://devblogs.microsoft.com/typescript/announcing-typescript-3-4-rc/

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@googlebot googlebot added the cla: yes label Mar 18, 2019

@filipesilva filipesilva force-pushed the filipesilva:ts-3.4 branch from 977f357 to 16112c9 Mar 18, 2019

@filipesilva

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2019

Similar to #29004

@mary-poppins

This comment has been minimized.

Copy link

commented Mar 18, 2019

@mary-poppins

This comment has been minimized.

Copy link

commented Mar 18, 2019

@filipesilva filipesilva force-pushed the filipesilva:ts-3.4 branch from 16112c9 to fb59648 Mar 18, 2019

@mary-poppins

This comment has been minimized.

Copy link

commented Mar 18, 2019

@filipesilva filipesilva force-pushed the filipesilva:ts-3.4 branch 2 times, most recently from 0c2da45 to f874d19 Mar 18, 2019

@mary-poppins

This comment has been minimized.

Copy link

commented Mar 18, 2019

@filipesilva filipesilva force-pushed the filipesilva:ts-3.4 branch from f874d19 to f240b7f Mar 18, 2019

@mary-poppins

This comment has been minimized.

Copy link

commented Mar 18, 2019

@filipesilva

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2019

@@ -91,7 +91,7 @@ export abstract class Renderer {
const renderedFiles: FileInfo[] = [];

// Transform the source files.
this.bundle.src.program.getSourceFiles().map(sourceFile => {
this.bundle.src.program.getSourceFiles().forEach(sourceFile => {

This comment has been minimized.

Copy link
@rkirov

rkirov Mar 18, 2019

Contributor

what happened here?

This comment has been minimized.

Copy link
@filipesilva

filipesilva Mar 18, 2019

Author Member

TS was showing an error here that the return value was not used: https://circleci.com/gh/angular/angular/248206

packages/compiler-cli/src/ngcc/src/rendering/renderer.ts:94:5 - error TS21222: return value is unused.
	See http://tsetse.info/check-return-value

 94     this.bundle.src.program.getSourceFiles().map(sourceFile => {
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 95       const compiledFile = decorationAnalyses.get(sourceFile);
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
... 
101       }
    ~~~~~~~
102     });

This comment has been minimized.

Copy link
@rkirov

rkirov Mar 19, 2019

Contributor

Interesting that TS3.4 will trigger that, it was not used before too.

This comment has been minimized.

Copy link
@filipesilva

filipesilva Mar 19, 2019

Author Member

Interesting indeed. Now that I look at it with fresh eyes, this isn't a TypeScript error proper, but a TseTse error: https://github.com/bazelbuild/rules_typescript/blob/b4bc468bfb03c05b68dde0b4f846bcd1a8fb7b01/internal/tsetse/tests/check_return_value/BUILD#L19

I wonder why TseTse wasn't catching this before.

cc @alexeagle

This comment has been minimized.

Copy link
@alexeagle

alexeagle Apr 3, 2019

Contributor

my guess is that we only look for usages of Array#map, while program.getSourceFiles() maybe was some other iterable type before like ReadonlyArray I guess now it became assignable to Array?

this.window.onerror(error);
} else {
this.window.onerror(error.message, undefined, undefined, undefined, error);
if (this.window.onerror) {

This comment has been minimized.

Copy link
@filipesilva

filipesilva Mar 19, 2019

Author Member

TS 3.4 was failing compilation with the error below https://circleci.com/gh/angular/angular/248678

ERROR in src/app/shared/reporting-error-handler.ts(32,7): error TS2721: Cannot invoke an object which is possibly 'null'.
src/app/shared/reporting-error-handler.ts(34,7): error TS2721: Cannot invoke an object which is possibly 'null'.

async function callOnError(message, url, line, column, error) {
await browser.executeScript(function() {
// reset the ga queue
(window as any).ga.q.length = 0;
// post the error to the handler
window.onerror(arguments[0], arguments[1], arguments[2], arguments[3], arguments[4]);
if (window.onerror) {

This comment has been minimized.

Copy link
@filipesilva

filipesilva Mar 19, 2019

Author Member

TS was failing compilation with the error below https://circleci.com/gh/angular/angular/248820

[18:29:57] E/launcher - Error: TSError: ⨯ Unable to compile TypeScript
tests/e2e/onerror.e2e-spec.ts (193,7): Cannot invoke an object which is possibly 'null'. (2721)
@@ -1,5 +1,5 @@
import { browser } from 'protractor';
import { SitePage } from './app.po';
import {browser} from 'protractor';

This comment has been minimized.

Copy link
@filipesilva

filipesilva Mar 19, 2019

Author Member

Saving this file triggered the clang auto format. I could also save without formatting, but that begs the question of why this wasn't formatted to begin with.

This comment has been minimized.

Copy link
@gkalpak

gkalpak Mar 19, 2019

Member

The files under aio/ are not formatted with clang-format (when using gulp format for example).
Some of the files definitely should not be formatted (e.g. aio/content/examples/, which should follow the Angular Style Guide) and although the app files themselves (in aio/src/) could be formatted, they historically haven't been 😁

(Note: The annoying auto-formatting will go away, once I find some time to clean-up #28842 😇)

This comment has been minimized.

Copy link
@filipesilva

filipesilva Mar 19, 2019

Author Member

Saved without formatting then, thanks for the context.

@filipesilva filipesilva force-pushed the filipesilva:ts-3.4 branch from f240b7f to b9bb9df Mar 19, 2019

@mary-poppins

This comment has been minimized.

Copy link

commented Mar 19, 2019

@ngbot ngbot bot added this to the needsTriage milestone Mar 19, 2019

@RinMinase

This comment has been minimized.

Copy link

commented Apr 3, 2019

@filipesilva Typescript 3.4.1 is out, but your package dependencies file states ~3.4.0-rc, would this be applicable to use ^3.4.1 or would this introduce another set of breaking changes?

@filipesilva

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

@RinMinase this PR was just an initial test when the RC came out to see how much work would be necessary on this repository. We will change it to support a stable version before merging.

@@ -157,7 +157,7 @@
"tree-kill": "^1.1.0",
"ts-node": "^3.3.0",
"tslint": "~5.9.1",
"typescript": "~3.3.3333",
"typescript": "~3.4.0-rc",

This comment has been minimized.

Copy link
@alexeagle

alexeagle Apr 5, 2019

Contributor

we are ready to merge this, could you update this to the released 3.4?

@ngbot

This comment has been minimized.

Copy link

commented Apr 5, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure conflicts with base branch "master"
    pending missing required labels: PR target: *
    pending status "google3" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@alexeagle

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

caretaker Igor: on your plate to clean this up and check the peerDep ranges everywhere

@filipesilva filipesilva force-pushed the filipesilva:ts-3.4 branch from b9bb9df to 86aebb3 Apr 6, 2019

@mary-poppins

This comment has been minimized.

Copy link

commented Apr 6, 2019

@Airblader Airblader referenced this pull request Apr 8, 2019

Closed

Support Typescript 3.4 #29766

@filipesilva filipesilva force-pushed the filipesilva:ts-3.4 branch from 86aebb3 to 5835c4a Apr 10, 2019

@filipesilva filipesilva marked this pull request as ready for review Apr 10, 2019

@filipesilva filipesilva requested review from angular/docs-infra as code owners Apr 10, 2019

@@ -5,7 +5,8 @@
"scripts": {
"build": "npm run clean && npm run ngc && npm run rollup && npm run rollup:lazy && npm run es5 && npm run es5:lazy",
"clean": "rm -rf dist",
"es5": "tsc --target es5 --skipLibCheck --allowJs dist/bundle.es2015.js --out dist/bundle.js",
"//es5-note": "`--typeRoots ./dummy_folder_name/@types` is a workaround for https://github.com/Microsoft/TypeScript/issues/30845",
"es5": "tsc --target es5 --skipLibCheck --allowJs dist/bundle.es2015.js --out dist/bundle.js --typeRoots ./dummy_folder_name/@types",

This comment has been minimized.

Copy link
@filipesilva

filipesilva Apr 10, 2019

Author Member

--typeRoots ./dummy_folder_name/@types is a workaround for microsoft/TypeScript#30845.

This test does not depend on @types/node, but they are present in the root node_modules, both via a direct dependency and transitively through the rollup dependency.

@mary-poppins

This comment has been minimized.

Copy link

commented Apr 10, 2019

@alexeagle
Copy link
Contributor

left a comment

thanks @filipesilva nice work 🌹

@IgorMinar
Copy link
Member

left a comment

Other than the one missed version everything else looks great. Thank you.

@@ -30,7 +30,7 @@ function testBazel() {
yarn
# Force more recent TS version until new Angular CLI projects also use it.
# --ignore-scripts is necessary because there is a postinstall script that uses ngc.
yarn add typescript@3.3.3333 --dev --ignore-scripts
yarn add typescript@3.4.0-rc --dev --ignore-scripts

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Apr 10, 2019

Member

Why not 3.4.2? Did you miss to update this file?

This comment has been minimized.

Copy link
@filipesilva

filipesilva Apr 10, 2019

Author Member

Yes, I missed this one, thank you for catching it 👍

Fixed now.

@filipesilva filipesilva force-pushed the filipesilva:ts-3.4 branch from 5835c4a to 34c815b Apr 10, 2019

@mary-poppins

This comment has been minimized.

Copy link

commented Apr 10, 2019

You can preview 34c815b at https://pr29372-34c815b.ngbuilds.io/.

@IgorMinar IgorMinar closed this in ef85336 Apr 10, 2019

wKoza added a commit to wKoza/angular that referenced this pull request Apr 17, 2019

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.