fix(upgrade): make ngUpgrade work with testability API #7603

sjelin · 2016-03-14T21:44:33Z

Please check if the PR fulfills these requirements
The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

(Docs not really relevant in this case)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

This change makes the testability API work with hybrid apps. Specifically, it fixes deferred bootstrapping and waiting on ng2 components within ng1 apps

What is the current behavior? (You can also link to an open issue here)

Currently deferred bootstrapping causes ngUpgrade to hang, and if an ng2 component uses setTimeout or the like the hybrid app's testability checks will totally ignore the fact that its ng2 components aren't yet stable

What is the new behavior (if this is a feature change)?

Now deferred bootstrapping works, and the testability API will check both ng1 and ng2 stability

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

Only if someone is relying on their hybrid app to crash!

Other information:

I have almost zero typescript knowledge so please tell me how to fix any style problems. I did compile/run tests though.

googlebot · 2016-03-14T21:44:34Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

sjelin · 2016-03-14T21:46:48Z

I signed it!

googlebot · 2016-03-14T21:46:50Z

CLAs look good, thanks!

sjelin · 2016-03-14T21:47:33Z

I've signed it before but you can have me resign any time you want @googlebot 😉

sjelin · 2016-03-14T22:37:44Z

cc @juliemr & whoever owns ngUpgrade

`upgrade.js` taken from angular/angular#7603

juliemr · 2016-03-14T22:52:47Z

modules/angular2/src/facade/lang.ts

We should probably add a comment that this will only be available if it's using ngUpgrade.

gkalpak · 2016-03-15T07:31:03Z

modules/angular2/src/upgrade/angular_js.ts

Why not add resumeBooystrap in the type above ?

It's there. clang just put it on the same line as version

Wow ! Did clang-format really do that ?
That's pretty unexpected !

_{(But then again you can't argue with clang-format.)}

I mean, I didn't put it on that line, so I think it must have been clang

sjelin · 2016-03-16T22:13:35Z

@mhevery ping

mhevery · 2016-03-17T04:16:48Z

modules/angular2/src/facade/lang.ts

Could we keep angular1 in ngUpgrade only.

I could probably do some casting to access the angular object without expanding the interface

mhevery · 2016-03-17T04:19:02Z

LGTM, except for the small nit-pick of keeping this to ngUpgraed only

sjelin · 2016-03-18T21:59:36Z

Having a lot of trouble rebasing this, hopefully I'll get it working later today

sjelin · 2016-03-19T00:08:30Z

Ok, rebased. It's breaking travis because of angular/zone.js#283, which I've submitted a PR for and hopefully will be fixed on npm soon. For now I've changed the implementation of zone.js in my node_modules and the unit tests do pass, at least locally.

Edit: it says MODE=js is passing but it does have errors: https://travis-ci.org/angular/angular/jobs/117041194#L3041 Must be a bug with our travis setup

mhevery · 2016-03-22T03:53:01Z

Rebasing on top of #7700 should make this Green.

sjelin · 2016-03-22T22:11:35Z

Rebasing fixed tests. All comments addressed.

mary-poppins · 2016-03-25T17:22:05Z

Merging PR #7603 on behalf of @kara to branch presubmit-kara-pr-7603.

`upgrade.js` taken from angular/angular#7603

angular-automatic-lock-bot · 2019-09-08T05:20:35Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: no label Mar 14, 2016

googlebot added cla: yes and removed cla: no labels Mar 14, 2016

sjelin force-pushed the ng-upgrade-testability branch from 171889a to 2d12af4 Compare March 14, 2016 21:53

sjelin added a commit to sjelin/protractor that referenced this pull request Mar 14, 2016

chore(tests): test for ngUpgrade support

278a52c

`upgrade.js` taken from angular/angular#7603

sjelin mentioned this pull request Mar 14, 2016

chore(tests): test for ngUpgrade support angular/protractor#3024

Merged

juliemr reviewed Mar 14, 2016
View reviewed changes

sjelin force-pushed the ng-upgrade-testability branch 2 times, most recently from afb1c45 to f637e6e Compare March 15, 2016 01:45

gkalpak reviewed Mar 15, 2016
View reviewed changes

juliemr added the action: review The PR is still awaiting reviews from at least one requested reviewer label Mar 15, 2016

juliemr assigned mhevery Mar 15, 2016

This was referenced Mar 15, 2016

Testing hybrid Angular 1+2 app always fail angular/protractor#2944

Closed

bug(ngUpgrade): modules provided to resumeBootstrap do not affect downgraded ng2 directives #7635

Closed

mhevery reviewed Mar 17, 2016
View reviewed changes

sjelin force-pushed the ng-upgrade-testability branch from f637e6e to 768871a Compare March 17, 2016 18:38

sjelin force-pushed the ng-upgrade-testability branch from 768871a to adebb72 Compare March 19, 2016 00:06

naomiblack added the action: review label Mar 22, 2016

fix(upgrade): make ngUpgrade work with testability API

9ee4cdb

sjelin force-pushed the ng-upgrade-testability branch from adebb72 to 9ee4cdb Compare March 22, 2016 21:05

naomiblack added the status: Travis OK label Mar 23, 2016

mhevery added pr_state: LGTM action: merge The PR is ready for merge by the caretaker labels Mar 25, 2016

mhevery assigned sjelin Mar 25, 2016

kara added the zomg_admin: do merge label Mar 25, 2016

mary-poppins removed the zomg_admin: do merge label Mar 25, 2016

mhevery closed this in 430f367 Mar 25, 2016

bradlygreen added this to the Angular 2 Release Candidate milestone Apr 4, 2016

sjelin mentioned this pull request Apr 6, 2016

fix(upgrade): support deferred bootstrap #7005

Closed

sjelin added a commit to sjelin/protractor that referenced this pull request Apr 28, 2016

chore(tests): test for ngUpgrade support

eb6e71d

`upgrade.js` taken from angular/angular#7603

damrbaby mentioned this pull request May 13, 2016

fix(upgrade): Ensure upgrade adapter works on angular.js 1.2 #8647

Merged

3 tasks

This was referenced May 22, 2016

Protractor E2E tests time out with NG1-NG2 hybrid bootstrapped application. #8758

Closed

Protractor E2E tests time out with NG1-NG2 hybrid bootstrapped application angular/protractor#3238

Closed

cnishina pushed a commit to angular/protractor that referenced this pull request Jun 2, 2016

chore(tests): test for ngUpgrade support (#3024)

3675198

`upgrade.js` taken from angular/angular#7603

petebacondarwin mentioned this pull request Dec 21, 2016

NgUpgrade bootstrap not resolving Ng1 rootScopePrototype when ng1 bootstrap is deferred #6547

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019

fix(upgrade): make ngUpgrade work with testability API #7603

fix(upgrade): make ngUpgrade work with testability API #7603

Uh oh!

Conversation

sjelin commented Mar 14, 2016

Uh oh!

googlebot commented Mar 14, 2016

Uh oh!

sjelin commented Mar 14, 2016

Uh oh!

googlebot commented Mar 14, 2016

Uh oh!

sjelin commented Mar 14, 2016

Uh oh!

sjelin commented Mar 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjelin commented Mar 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhevery commented Mar 17, 2016

Uh oh!

sjelin commented Mar 18, 2016

Uh oh!

sjelin commented Mar 19, 2016

Uh oh!

mhevery commented Mar 22, 2016

Uh oh!

sjelin commented Mar 22, 2016

Uh oh!

mary-poppins commented Mar 25, 2016

Uh oh!

angular-automatic-lock-bot bot commented Sep 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants