X Tutup
Skip to content

fix(http): allow using jsonpInjectables and httpInjectables in same i…#3390

Closed
0x-r4bbit wants to merge 1 commit intoangular:masterfrom
0x-r4bbit:fix-http
Closed

fix(http): allow using jsonpInjectables and httpInjectables in same i…#3390
0x-r4bbit wants to merge 1 commit intoangular:masterfrom
0x-r4bbit:fix-http

Conversation

@0x-r4bbit
Copy link
Copy Markdown
Contributor

…njector

Fixes #3365

@0x-r4bbit
Copy link
Copy Markdown
Contributor Author

/cc @jeffbcross

@0x-r4bbit 0x-r4bbit force-pushed the fix-http branch 2 times, most recently from 10327ce to 0e51677 Compare July 30, 2015 14:38
@jeffbcross jeffbcross self-assigned this Jul 30, 2015
@jeffbcross jeffbcross added this to the alpha-34 milestone Jul 30, 2015
@jeffbcross
Copy link
Copy Markdown
Contributor

Nice!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should remove the ConnectionBackend import

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeffbcross done!

@jeffbcross
Copy link
Copy Markdown
Contributor

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?

@0x-r4bbit
Copy link
Copy Markdown
Contributor Author

@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 :)

@jeffbcross
Copy link
Copy Markdown
Contributor

@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.

@jeffbcross
Copy link
Copy Markdown
Contributor

Adding XHRBackend and JSONPbackend respectively inside the injectables lists will fix the E2E issue

@0x-r4bbit
Copy link
Copy Markdown
Contributor Author

Okay, I'll have a look at this.

@tbosch tbosch added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 30, 2015
@jeffbcross jeffbcross modified the milestones: alpha-35, alpha-34 Aug 3, 2015
@0x-r4bbit
Copy link
Copy Markdown
Contributor Author

I'm back on this now, hold your horses

@0x-r4bbit
Copy link
Copy Markdown
Contributor Author

@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?

@0x-r4bbit
Copy link
Copy Markdown
Contributor Author

I think I also need some help on how to spy on a backends createConnection method. Both backends are bound to MockBackend but that one doesn't use SpyObject. Any hint?

@jeffbcross
Copy link
Copy Markdown
Contributor

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?

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?

@jeffbcross jeffbcross modified the milestones: alpha-36, alpha-35 Aug 12, 2015
@0x-r4bbit
Copy link
Copy Markdown
Contributor Author

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?)

@0x-r4bbit
Copy link
Copy Markdown
Contributor Author

There you go. Thank you so much @jeffbcross !

@0x-r4bbit
Copy link
Copy Markdown
Contributor Author

Updated PR to use clang. This should make Travis happy.

@0x-r4bbit 0x-r4bbit mentioned this pull request Aug 17, 2015
35 tasks
@jeffbcross jeffbcross added pr_state: LGTM action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Aug 17, 2015
@mary-poppins
Copy link
Copy Markdown

Merging PR #3390 on behalf of @jeffbcross to branch presubmit-jeffbcross-pr-3390.

@mary-poppins mary-poppins removed the action: merge The PR is ready for merge by the caretaker label Aug 17, 2015
@jeffbcross
Copy link
Copy Markdown
Contributor

Merging, thanks!

@mhevery mhevery added state: Periodic Review action: merge The PR is ready for merge by the caretaker and removed state: Periodic Review action: merge The PR is ready for merge by the caretaker labels Aug 20, 2015
@mhevery
Copy link
Copy Markdown
Contributor

mhevery commented Aug 24, 2015

@jeffbcross Is this merged? Can you check?

@jeffbcross
Copy link
Copy Markdown
Contributor

@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!

@angular-automatic-lock-bot
Copy link
Copy Markdown

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(http): cannot use JSONP_BINDINGS and HTTP_BINDINGS in same injector

6 participants

X Tutup