-
-
Notifications
You must be signed in to change notification settings - Fork 85
Add mixin for Ember Data integration #114
Add mixin for Ember Data integration #114
Conversation
|
This looks great. We need to describe the migration path for people who're looking to adopt this. It can serve as a transition path towards using Ember Ajax with Ember Data. Primarily, we need to show how error handling needs to be adjusted to work with Ember Ajax in Ember Data. On that note, we'll need to add Ember Data Error object detection to our error detection helpers. |
|
Yeah, agreed. Definitely need to add some docs, but I'd rather not just add more to the README... There's a lot of information in there already. Maybe usage example in the mixin file? I can add some of that. What's the right way to add detection of the Ember Data errors? We'd need to import the errors and then do an |
d45c91c to
77046c9
Compare
|
What do you think about moving the mixin to |
|
I think that's kind of confusing, having a mixin in the |
|
That's kinda how Ember Data does it, Not exactly, but similar https://github.com/emberjs/data/blob/master/addon/-private/adapters/build-url-mixin.js |
|
I see what you're saying... But that's also a "private" module, not public. To me, addons have a given structure and I think adhering to the convention is a better way to go. |
|
@alexlafroscia ok, what do you think about putting the upgrade instructions into the README for now and later we can give it a section on a documentation site. |
@alexlafroscia that's a great point. I'll think about this today and get back to you. |
|
I'm wondering if it's really necessary to add the ED error checkers... I mean, if they're opting into using the Ember AJAX mixin, they need our error checkers for network errors. If the ED networking code isn't being run, how do those errors crop up? |
|
Yeah, you're right. Wi this strategy, it's not necessary because the assumption is that we will be running all of the Ajax code through Ember Ajax with the mixin. We should make this available as beta and have people try it. |
|
Cool. I'll add a commit with some docs and we can cut a release with this. Would you want to include this as part of |
|
Let's stick within 2.x series for now. So, 2.5-beta.1? |
|
@taras Can I get one last check-over before pulling this? |
addon/mixins/ajax-support.js
Outdated
|
|
||
| host: alias('ajaxService.host'), | ||
|
|
||
| namespace: alias('ajaxService.host'), |
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 should alias namespace, right?
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.
Good find, thanks!
|
@taras can I get one last once-over on this before merging? I fixed that thing you caught. |
README.md
Outdated
| const { JSONAPIAdapter } = DS; | ||
|
|
||
| export default JSONAPIAdapter.extend(AjaxServiceSupport, { | ||
| ajaxService: service('ajax'), |
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.
The mixin does this, we don't need this line and we need to move `namespace: 'api'`` to the service. Right?
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.
RE: the ajaxService line, I know we don't need it. I just wanted to illustrate that you can specify the service to use, but maybe that was confusing? What do you think @taras?
RE: the namespace line, I wanted to show how you can specify certain things on the adapter. Otherwise, the whole adapter is just
export default JSONAPIAdapter.extend(AjaxServiceSupport);which isn't much to show, really... Although maybe that's okay? What do you think?
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.
Yeah, I think that's fine. We want to show people that they can allow ember-ajax to take care of everything related to the request.
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.
Alright, I'll update the example code to show a "minimal" example, and also include an "extended" one that's similar to what I originally put. Sounds good?
|
Other than the 1 comment, it's good to go. |
de25df9 to
b7ed4ba
Compare
|
This appears to add the namespace twice: first in the ember-data adapter and again in the ember-ajax service. |
|
@dwickern can you make a new big ticket with some more information? |
@taras and I worked on this PR together, to introduce a mixin that can be added to an Ember Data adapter. This should be seen as a first step toward solving #85.
When included, the mixin will use the
ajaxservice for networking, instead of the code provided as part of the RestAdapter. Since theember-ajaxcode was based on the code that it is now replacing, bridging the gap here was pretty straightforward.The one thing that I'm not sure about right now is the naming of file that the mixin is found in. I think having it simply called
ajax-supportis a little ambigious, especially when the implementation file is calledajax-request. They're just a little too similar. I'd love suggestions on a better name.