refactor(ngUpgrade): Small cleanup with Testability API and resumeBootstrap #12926
Conversation
| @@ -8,6 +8,7 @@ | |||
| import {Component, Directive, DoCheck, ElementRef, EventEmitter, Inject, Injectable, Injector, Input, NgModule, OnChanges, OnDestroy, OnInit, Output, SimpleChanges} from '@angular/core'; | |||
| import {BrowserModule} from '@angular/platform-browser'; | |||
| import {platformBrowserDynamic} from '@angular/platform-browser-dynamic'; | |||
| import * as angular from '@angular/upgrade/src/angular_js'; | |||
There was a problem hiding this comment.
We shouldn't need this to build this app.
There was a problem hiding this comment.
In fact it causes Travis to fail.
There was a problem hiding this comment.
I was running into errors when I didn't have it there but I'll try again
petebacondarwin
added
comp: upgrade
effort1: hours
freq2: medium
action: cleanup
sjelin
force-pushed
the
static-upgrade
branch
from
cb92ecc to
cd7a74d
Compare
| @@ -115,7 +115,7 @@ class Ng2AppModule { | |||
| // #docregion Angular 1 Stuff | |||
| // #docregion ng1-module | |||
| // This Angular 1 module represents the Angular 1 pieces of the application | |||
| const ng1AppModule = angular.module('ng1AppModule', []); | |||
| const ng1AppModule = (window as any).angular.module('ng1AppModule', []); | |||
There was a problem hiding this comment.
@petebacondarwin I was still getting errors about typescript not knowing about the name angular so I did this instead
There was a problem hiding this comment.
where are you getting errors? In the editor or in the build?
There was a problem hiding this comment.
I just merged a fix for this. Please remove the any cast and verify that this error is now no longer in the editor as well as not being reported when running ./test.sh.
There was a problem hiding this comment.
During the build process of the tests:
sjelin@sjelin-linux:~/angular$ ./test.sh browserNoRouter
Watching: modules/tsconfig.json in /usr/local/google/home/sjelin/angular
=====> node_modules/.bin/tsc --emitDecoratorMetadata --project modules/tsconfig.json --watch
=====> node node_modules/karma/bin/karma start --no-auto-watch --port=9876 karma-js.conf.js
modules/@angular/examples/upgrade/static/ts/module.ts(118,22): error TS2304: Cannot find name 'angular'.
1:44:56 PM - Compilation complete. Watching for file changes.
------------------------------------------------------------------------------
Errors found.... (response not triggered)
I use vim so my editor doesn't provide errors
…tstrap * With non-static ngUpgrade apps, callbacks to `whenStable` were being invoked with the wrong context * With non-static ngUpgrade apps, `resumeBootstrap` was being run outside the NgZone * Remove redundent `whenStableContext` variable Neither of the first two problems were actually causing bugs (as far as I know), but they *might* have caused problems in the future. Inspired by angular#12910, but for non-static apps.
sjelin
force-pushed
the
static-upgrade
branch
from
cd7a74d to
d61433f
Compare
|
Should be good now @petebacondarwin. Thanks to @chuckjaz for the fix |
petebacondarwin
added
action: merge
|
Hey, this isn't merging for some reason and I don't have the rights to merge it myself |
|
It is up to the caretaker to merge it. Try pinging them on Slack |
|
@petebacondarwin This is missing an LGTM so it didn't show up in my query of PRs ready to merge. |
|
OK. Added. |
…tstrap (#12926) * With non-static ngUpgrade apps, callbacks to `whenStable` were being invoked with the wrong context * With non-static ngUpgrade apps, `resumeBootstrap` was being run outside the NgZone * Remove redundent `whenStableContext` variable Neither of the first two problems were actually causing bugs (as far as I know), but they *might* have caused problems in the future. Inspired by #12910, but for non-static apps.
|
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. |
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x")
What is the current behavior? (You can also link to an open issue here)
See "Other Information"
What is the new behavior?
See "Other Information"
Does this PR introduce a breaking change? (check one with "x")
Other information:
whenStablewere being invoked with the wrongcontext
resumeBootstrapwas being run outside the NgZonewhenStableContextvariableNeither of the first two problems were actually causing bugs (as far as I know), but they might
have caused problems in the future,
Inspired by #12910, but for non-static apps.