fix(http): allow using jsonpInjectables and httpInjectables in same i…#3390
fix(http): allow using jsonpInjectables and httpInjectables in same i…#33900x-r4bbit wants to merge 1 commit intoangular:masterfrom
Conversation
|
/cc @jeffbcross |
10327ce to
0e51677
Compare
|
Nice! |
modules/angular2/http.ts
Outdated
There was a problem hiding this comment.
Should remove the ConnectionBackend import
|
I'm going to pull down locally and investigate breakages from the previous Travis run. I think they were just in E2E tests (plus a dartanalyzer failure that will be fixed since you removed the unused ConnectionBackend import). Could you add a unit test with an injector that is created with both sets of injectables, and then verify that both backend.createConnections are called as expected? |
|
@jeffbcross ha! in fact I started with writing tests and then wasn't sure how to test the backend connection. But yea, I'll try it again :) |
|
@PascalPrecht probably easiest to create a parent injector binding the backends to mocks, and then a child injector that just contains the two sets of injectables. |
|
Adding XHRBackend and JSONPbackend respectively inside the injectables lists will fix the E2E issue |
|
Okay, I'll have a look at this. |
|
I'm back on this now, hold your horses |
|
@jeffbcross When binding both backends to a mock backend in a parent injector, whereas the child injector still binds the http and jsonp injectables (which have bindings for XHRBackend and JSONPBackend too), don' they shadow the mock bindings from the parent injector? |
|
I think I also need some help on how to spy on a backends |
If you use injectables in a child injector, it would override bindings in the parent injector since it includes bindings for the backends. Maybe we could pair/vc sometime tomorrow to work this out? |
|
Correct, that's why I was asking. Because Then it doesn't really make sense to have a parent injector that binds to mock backends, if there's a child injector that overrides those anyway. Sure, let's pair on that tomorrow. Ping me on Hangouts (or even Slack?) |
|
There you go. Thank you so much @jeffbcross ! |
|
Updated PR to use clang. This should make Travis happy. |
|
Merging PR #3390 on behalf of @jeffbcross to branch presubmit-jeffbcross-pr-3390. |
|
Merging, thanks! |
|
@jeffbcross Is this merged? Can you check? |
|
@mhevery it didn't merge because a conflict got merged ahead of it. I fixed the conflict and re-pushed to presubmit branch. Thanks for pointing it out! |
|
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. |
…njector
Fixes #3365