-
Notifications
You must be signed in to change notification settings - Fork 122
change Http to HttpClient #96
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
Conversation
f943ad6 to
346e615
Compare
|
Any chance this gets merged soon? |
HennerM
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late response.
The change to HttpClient is appreciated, after you have addressed my comments I am happy to merge it publish it as part of the 4.0 release, because this is a breaking change.
package.json
Outdated
| "repository": { | ||
| "type": "git", | ||
| "url": "https://github.com/ghidoz/angular2-jsonapi" | ||
| "url": "https://github.com/agustinvinao/angular2-jsonapi" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you revert this change please?
package.json
Outdated
| "license": "MIT", | ||
| "bugs": { | ||
| "url": "https://github.com/ghidoz/angular2-jsonapi/issues" | ||
| "url": "https://github.com/agustinvinao/angular2-jsonapi/issues" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this url as well
| @@ -0,0 +1,10 @@ | |||
| export function HasOne(config: any = {}) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What distinguishes HasOne From `BelongsTo´ ?
| @@ -0,0 +1,40 @@ | |||
| // import {BOOK_PUBLISHED, BOOK_TITLE} from "./author.fixture"; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file still needed?
| body.data.forEach((data: any) => { | ||
| let model: T = new modelType(this, data); | ||
| let model: T; | ||
| body.data.map((_data: any) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this bet written as
´models = body.data.map( ... ) andreturn model` in the map function?
| let requestHeaders = new Headers(); | ||
| requestHeaders.set('Accept', 'application/vnd.api+json'); | ||
| requestHeaders.set('Content-Type', 'application/vnd.api+json'); | ||
| if (this._headers) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this means the user can not set "global" headers anymore. Was this intended?
|
Im working to check your comments. I'll update the PL as soon as I can. I've forked the repo and Im using it with this changes, i need to see if all your comments still work on my current project, mainly see if the globals headers works ok. |
|
@agustinvinao you are changing a lot of different things in this pull request. It will be a lot easier and safer for merging if you focus on one change at a time. Can you please create a few smaller pull requests? |
| "name": "angular2-jsonapi", | ||
| "version": "3.6.0", | ||
| "description": "A lightweight Angular 2 adapter for JSON API", | ||
| "name": "angular2-jsonapi-adapter", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not change the name, another issue is opened for that matter.
HttpClient implementation. Part of the changes needs to update from Headers to HttpHeaders.