Skip to content
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

Add new basic adapter and serializer. #11

Merged
merged 3 commits into from
Nov 1, 2014
Merged

Add new basic adapter and serializer. #11

merged 3 commits into from
Nov 1, 2014

Conversation

benkonrath
Copy link
Collaborator

Here's my start of the re-write. Some notes ...

  • The adapter / serializer are not hooked up as an addon because I don't know how to do it properly yet but I wanted to get my code out for you to look at.
  • Get is working in the project I'm using to test / develop this. There will probably be some more changes for full CRUD but I'm working on it.
  • I looked at the files that Ember CLI generated for a new addon and there are a bunch of files that we might want to pull in (including a setup for testing but I'm not sure if you want to use that configuration).
  • I have a solution for reducing the number of queries for hasMany relationships since coalesceFindRequests doesn't work with DRF but I want to get this working first before I add more code on.

It would be helpful if you could show me how to wire this code properly in. If you're not sure as well, I can spend some time reading the Ember CLI docs a little more. Comments are appreciated. Feel free to wipe out or integrate this commit into another one. This is very much a work in progress and doesn't need to be preserved in the repo history.

@g-cassie
Copy link

I'm not great with ember-cli so I'm going to wait for someone else to suggest how to plug this new code in easily so I can give it a whirl on my own project.

From looking over the code in your commit I have a couple questions:

  1. Why dasherize pathForType? I know this is from the existing adapter but I think that django-rest-framework actually defaults to the lower case version of the name (e.g. FruitBasket -> fruitbasket). (See here)[https://github.com/tomchristie/django-rest-framework/blob/master/rest_framework/routers.py#L128]. This becomes confusing if you add nested endpoints in a ViewSet using the @detail_route decorator as it is impossible to have dashes in a python function name.
  2. Can you simply use the normalizePayload() hook instead of overriding extractArray?

@benkonrath
Copy link
Collaborator Author

Point 1: The existing adapter uses underscorize, I changed to using dasherize because slugs in Django use dashes and that's how I setup my API URLs usually. But matching the defaults in DRF is a better strategy so this is a mistake and I'll update the PR shortly. Thanks for pointing this out.

Point 2: I overrode extractArray instead of normalizePayload because normalizePayload is called for both single items and multiple items. extractArray is only called for multiple items and results is only expected to be present for multiple items so it seems like the right place. Here's the comment for extractArray from the RESTSerializer:

Called when the server has returned a payload representing multiple records, such as in response to a findAll or findQuery.

It is your opportunity to clean up the server's response into the normalized form expected by Ember Data.

If you want, you can just restructure the top-level of your payload, and do more fine-grained normalization in the normalize method.
...

Can you see any problems with this reasoning?

@benkonrath
Copy link
Collaborator Author

I just pushed a new version of the commit with lower case model name as the default. I also updated the README to reflect the change.

@dustinfarris
Copy link
Owner

@benkonrath nice start! I need to take more time to give this a thorough look over, but I think we are on the right track here.

@g-cassie
Copy link

@benkonrath Point 2 makes sense. I actually misread the DRF code on point 1 - DRF does not specify a default url scheme. I still think that dashes are a bad idea because as I mention you cannot make custom api endpoints with dashes. Thanks for taking my suggestion.

I have a repo up here with my very early workings on a DRF side adapter. Right now only the output side is written - input to come tomorrow! Any feedback would be very much appreciated.

@benkonrath
Copy link
Collaborator Author

@g-cassie Clearly I mis-read the code as well. I've updated the README to correct the problem but I still agree with the change.

@dustinfarris Great! Let me know if you want me to adjust anything - like move the docs to a different location, etc. I can squash the commits in the pull request before you merge it - no sense in having a polluted git history. Just let me know.

dustinfarris added a commit that referenced this pull request Nov 1, 2014
Add new basic adapter and serializer.
@dustinfarris dustinfarris merged commit 3b792d1 into dustinfarris:version-1.0 Nov 1, 2014
@dustinfarris
Copy link
Owner

@benkonrath not worried about the git history at the moment, in fact I'd rather have more details than less in this early stage. I merged your PR so I can start working on this myself. Ping me in IRC when you have a chance to discuss.

@holandes22
Copy link
Collaborator

Hi @dustinfarris @benkonrath I wanna help out with the re-write, do you have like a trello board or similar where I can see the remaining tasks? Or maybe I can set one up, so we can assign tasks more easily and track their progress. What is the IRC channel so I can get involved?

@benkonrath benkonrath deleted the version-1.0 branch November 4, 2014 12:27
@benkonrath
Copy link
Collaborator Author

@holandes22 Great to hear. I think that github issues is a good way to keep track of the tasks. The first task is getting the tests up and running which I'm working on now. Once that's done, I'll create a bunch of issues for things that need to be done and you can pick up anything that interests you. Just 'watch' this repo so you get notifications of the issues and PRs. Thanks.

@holandes22
Copy link
Collaborator

@benkonrath thanks for the update :)
Is there anything specific you need help with at this moment?
I was planning on testing the branch on my app tonight, to see what issues are there, and work on those. Is it usable as this point? anything I should know before installing it?

@benkonrath
Copy link
Collaborator Author

@holandes22 I've tested the new adapter with list and detail retrieve. coalesceFindRequests doesn't work with DRF so you should expect one request for every item in a list retrieve. I already have a solution in the pipe for this so it's not worth spending time on this particular issue.

Here's a couple of tasks that I haven't done yet:

  • It would be great if you could check if create, update & delete are working.
  • I didn't check if the pagination is working so you could check that out as well or instead.

If something doesn't work, you can file an issue but also try to fix it. Please keep the changes as small as possible because I really want to follow / think along to make sure we're meeting the re-write goals.

Thanks!

@holandes22
Copy link
Collaborator

Sounds good, I'll check update/create/delete and see how it goes.

Regarding pagination, is that supposed to work? DRF returns a custom format for pagination that ED does not understand and I didn't see anything in the adapter that should handle this

@benkonrath
Copy link
Collaborator Author

@holandes22 extractMeta is the start of the pagination:

https://github.com/dustinfarris/ember-django-adapter/blob/version-1.0/addon/serializers/drf.js#L6

I'm sure there's more to it though. I have to read the ED docs a little more to see how it should work. Looking at update/create/delete is probably the best for right now.

@holandes22
Copy link
Collaborator

I see, thanks for clarifying that. I'll start by checking crud operations as I don't use pagination in my API yet. I'll keep you guys posted

@dustinfarris
Copy link
Owner

@holandes22 thanks for helping out. I'm on #ember-django-adapter if you need anything.

@benkonrath
Copy link
Collaborator Author

Just a quick note to report that delete is working with this code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants