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.

Sign up for GitHub

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

Break buildURL into multiple requestType methods #2966

Merged
merged 1 commit into from
Apr 10, 2015
Merged

Break buildURL into multiple requestType methods #2966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 114 additions & 6 deletions packages/ember-data/lib/adapters/build-url-mixin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,42 @@ export default Ember.Mixin.create({

@method buildURL
@param {String} type
@param {String|Array} id single id or array of ids
@param {String|Array|Object} id single id or array of ids or query
@param {DS.Snapshot|Array} snapshot single snapshot or array of snapshots
@param {String} requestType
@return {String} url
*/
buildURL: function(type, id, snapshot, requestType) {
switch (requestType) {
case 'find':
return this.urlForFind(id, type, snapshot);
case 'findAll':
return this.urlForFindAll(type);
case 'findQuery':
return this.urlForFindQuery(id, type);
case 'findMany':
return this.urlForFindMany(id, type, snapshot);
case 'findHasMany':
return this.urlForFindHasMany(id, type);
case 'findBelongsTo':
return this.urlForFindBelongsTo(id, type);
case 'createRecord':
return this.urlForCreateRecord(type, snapshot);
case 'deleteRecord':
return this.urlForDeleteRecord(id, type, snapshot);
default:
return this._buildURL(type, id);
}
},

/**
@method _buildURL
@private
@param {String} type
@param {String} id
@return {String} url
*/
_buildURL: function(type, id) {
var url = [];
var host = get(this, 'host');
var prefix = this.urlPrefix();
Expand All @@ -59,11 +89,7 @@ export default Ember.Mixin.create({
if (path) { url.push(path); }
}

//We might get passed in an array of ids from findMany
//in which case we don't want to modify the url, as the
//ids will be passed in through a query param
if (id && !Ember.isArray(id)) { url.push(encodeURIComponent(id)); }

if (id) { url.push(encodeURIComponent(id)); }
if (prefix) { url.unshift(prefix); }

url = url.join('/');
Expand All @@ -72,6 +98,88 @@ export default Ember.Mixin.create({
return url;
},

/**
* @method urlForFind
* @param {String} id
* @param {String} type
* @param {DS.Snapshot} snapshot
* @return {String} url
*/
urlForFind: function(id, type, snapshot) {
return this._buildURL(type, id);
},

/**
* @method urlForFindAll
* @param {String} type
* @return {String} url
*/
urlForFindAll: function(type) {
return this._buildURL(type);
},

/**
* @method urlForFindQuery
* @param {Object} query
* @param {String} type
* @return {String} url
*/
urlForFindQuery: function(query, type) {
return this._buildURL(type);
},

/**
* @method urlForFindMany
* @param {Array} ids
* @param {String} type
* @param {Array} snapshots
* @return {String} url
*/
urlForFindMany: function(ids, type, snapshots) {
return this._buildURL(type);
},

/**
* @method urlForFindHasMany
* @param {String} id
* @param {String} type
* @return {String} url
*/
urlForFindHasMany: function(id, type) {
return this._buildURL(type, id);
},

/**
* @method urlForFindBelongTo
* @param {String} id
* @param {String} type
* @return {String} url
*/
urlForFindBelongsTo: function(id, type) {
return this._buildURL(type, id);
},

/**
* @method urlForCreateRecord
* @param {String} type
* @param {DS.Snapshot} snapshot
* @return {String} url
*/
urlForCreateRecord: function(type, snapshot) {
return this._buildURL(type);
},

/**
* @method urlForDeleteRecord
* @param {String} id
* @param {String} type
* @param {DS.Snapshot} snapshot
* @return {String} url
*/
urlForDeleteRecord: function(id, type, snapshot) {
return this._buildURL(type, id);
},

/**
@method urlPrefix
@private
Expand Down
2 changes: 1 addition & 1 deletion packages/ember-data/lib/adapters/rest-adapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,7 @@ export default Adapter.extend(BuildURLMixin, {
@return {Promise} promise
*/
findQuery: function(store, type, query) {
var url = this.buildURL(type.typeKey, null, null, 'findQuery');
var url = this.buildURL(type.typeKey, query, null, 'findQuery');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this related to the PR in question?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for passing query along to buildURL; I will find that useful!

I'm wondering, though, why is it passed as the id parameter and would it make more sense as the snapshot parameter? I know it's not really a snapshot, but it seems more like a snapshot than an id. Before this change, id is either null, a string, or an array of strings. With this change it can also be an object. snapshot on the other hand, is either null, an object, or an array of objects. Also, I think the query is used similarly as the snapshot (to fill arbitrary values in the url).

For reference, here is the signature of buildURL:

buildURL: function(type, id, snapshot, requestType) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be a breaking change for anyone currently overriding buildURL that is expecting it to be an actual snapshot. And I would say that the breaking change should be adding a formal query argument so that the signature would be:

buildURL: function(type, id, query, snapshot, requestType) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok
I just thought I would check. I don't understand how putting the using query instead of null would break anything that using query instead of id would, but it sounds like you've thought about this more in depth than I have.

Anyway, I'm just happy to have the query available in buildURL at all. Thanks!


if (this.sortQueryParams) {
query = this.sortQueryParams(query);
Expand Down