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

Support trailing slashes on urls for coalesceFindMany. #2468

Closed
wants to merge 1 commit into from

Conversation

g-cassie
Copy link

Currently if you override your adapter as follows, coalesceFindRequests will break (silently).

export default DS.ActiveModelAdapter.extend({
    buildURL: function(type, id, record){
      return this._super(type, id, record) + '/';
    },
});

As _stripIDFromURL is marked as a private method I do not think it should be expected that users override it. This provides a simple fix. Happy to add a test if someone can suggest a simple approach for testing this.

I've noticed the build is failing. I'll wait to hear whether this would be a merge candidate before fixing. I will also need to make some adjustments to make sure that coalesce uses a trailing slash in the url if a trailing slash url is already being used.

@igorT
Copy link
Member

igorT commented Nov 24, 2014

We already have some tests in the rest adapter tests, that would be a good place to put them. Why does your adapter return a trailing slash? Not 100% sure we should support this but seems ok. @jcollins1991 If you rebase of master the build should pass. Thanks! Sorry for the late reply

@g-cassie
Copy link
Author

Urls in Django have a trailing slash by default so I don't think this is a niche use case. I will fix up the PR and add a test later this week.

@dustinfarris
Copy link

What is keeping this from being merged?

@bmac
Copy link
Member

bmac commented Jan 25, 2015

Looks like it needs a test case.

@dustinfarris
Copy link

@g-cassie are you still working on tests for this?

@g-cassie
Copy link
Author

Sorry, I won't have any time to look at this again for at least a month. I don't have much experience with Ember testing either.

@dustinfarris
Copy link

@g-cassie ok, if you don't mind, I'm going to finish it then. Need this for ember-django-adapter.

@g-cassie
Copy link
Author

@dustinfarris please do. Sorry I can't be of more help right now.

@dustinfarris
Copy link

So I ended up overriding this in our custom adapter, which I believe is the right way forward. This can be closed without merging in my opinion.

However, in doing so, I decided to greatly simplify the method by proxying to buildUrl. These seems to have the same effect with much less effort. Am I missing something?

https://github.com/dustinfarris/ember-django-adapter/blob/master/addon/adapters/drf.js#L132-L134

@igorT
Copy link
Member

igorT commented Jun 9, 2015

@dustinfarris if buildUrl works, i think it's only on accident

@igorT igorT closed this Jun 9, 2015
@benkonrath
Copy link

@igorT Can you be more specific about the problem with @dustinfarris's solution? Do you think it would be better to add tests for code in this PR and get it merged into ember-data?

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.

5 participants