amiel / ember-data-url-templates

an ember-addon to allow building urls with url templates instead of defining buildURL
https://github.com/amiel/ember-data-url-templates/wiki
MIT License
129 stars 22 forks source link

query keys in query string #19

Closed Dhaulagiri closed 8 years ago

Dhaulagiri commented 8 years ago

The docs for query show how to include just certain keys in a request, but in my use I am still seeing the query keys being included in the query string. For example:

// example request and template
this.store.query('app', { 'appName': 'foo' })
queryUrlTemplate: '{+host}/apps/{appName}/app'

Yields this url:

https://foo.com/apps/foo/app?appName=foo

When I really want just this:

https://foo.com/apps/foo/app

Are the docs incorrect or is there something I'm doing incorrectly here?

amiel commented 8 years ago

@Dhaulagiri Yes, this is an issue. Unfortunately, ember-data appends the query string after the url is built with buildURL.

I kind of think this is a bug in ember-data but would like to check with the core team and find out if this is intentional.

There is a workaround. The idea is to make sure the query object ends up being empty. You can do this by deleting appName from the query object in a urlSegment. For example:

// adapter
export default ApplicationAdapter.extend(UrlTemplates, {
  queryUrlTemplate: '{+host}/apps/{appName}/app'

  urlSegments: {
    appName(type, id, snapshot, query) {
      var appName = query.appName;
      delete query.appName;
      return appName;
    },
  },
});
amiel commented 8 years ago

@igorT do you have any thoughts on if it makes sense that the default adapter is appending the query string to the result from buildURL or if that should only happen in buildURL?

cfator commented 8 years ago

I agree Ember.Data lets you construct the URL but not the query params which seems really strange.

I ended up using RESTAdapter.sortQueryParams to achieve this.

amiel commented 8 years ago

@cfator oh, would you be willing to share your solution?

kehphin commented 8 years ago

I think this will work:

export default ApplicationAdapter.extend(UrlTemplates, {
  queryUrlTemplate: '{+host}/apps/{appName}/app',

  urlSegments: {
    appName(type, id, snapshot, query) {
      return query.appName;
    }
  },

  sortQueryParams: function(params) {
    return {};
  }
});

Keep in mind that you might be doing other developers a disservice when they try to add a query param to this code and are left pondering why it doesn't work... heh

amiel commented 8 years ago

@kehphin Nice! I wonder if it would be worth including sortQueryParams in the UrlTemplate mixin.

Theoretically, it shouldn't be a problem. If one wants to include query params with url templates, they should define them as part of the template with queryUrlTemplate: '{+host}/apps/{appName}/app?{?query*}'.

However, I agree that this could be confusing and some people might even be depending on the existing functionality. My other concern here is that might only be compatible with a few versions of ember-data.

I'd love some input here.

OandrewO commented 8 years ago

@amiel : Did you find out if this was an intended behavior on the Ember Data teams' part? I was able to construct the correct endpoint by following the workarounds in this thread, but they seem very hacky.

amiel commented 8 years ago

@OandrewO I have not heard from the Ember Data team on this, and I agree, the workarounds are hacky. I'll try pinging them again on slack.

amiel commented 8 years ago

@OandrewO / others: It looks like https://github.com/emberjs/data/pull/3099 will provide the hook we need to customize the query params (dataForRequest).

Once that gets un-feature flagged and I personally have an app that is caught up to that version, I will be happy to work on having ember-data-url-templates use that hook to prevent this issue.

If anyone else has an app up to canary and wants to work on it, please do :)

For now, looking through the source, using sortQueryParams seems like the best short-term solution.

amiel commented 8 years ago

Ok, I just released 0.2.0 with this hack.

With this change, you should be able to remove either hack (sortQueryParams, or the delete hack).

Note that if you were using sortQueryParams for another reason, or depend on the fact that the query params were already appended, this could cause unintended changes.

misteroh commented 8 years ago

awesome! thanks!!