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

The Model can accept the scope to do the url scoping, especially useful for nested resources. #410

Closed
wants to merge 2 commits into from

Conversation

sishen
Copy link
Contributor

@sishen sishen commented Feb 13, 2013

The scope can either be a function or string. The generated url will be "scope/resources".

For example,

Ticket.scope = "/projects/1"
Ticket.url() ->  "/projects/1/tickets"
new Ticket({id: 1}).url() -> "/projects/1/tickets/1"

Ticket.prototype.scope = function() { return "/projects/" + this.project_id }
new Ticket({id: 1, project_id: 2}).url() -> "/projects/2/tickets/1"

either be a function or string. The generated url will be "scope/resources"
@adambiggs
Copy link
Member

+1

I currently have to hack @url() for each model to achieve this.

@cengebretson
Copy link
Member

I think this looks like something that could be very useful. In the pull request it appears this is applied only at the model/collection level. Should we take this a step further and allow instances to also define their own scope?

Seems like that would solve problems like #412. In this case the user wants to have an ajax request that looks like /photo/{photo_id}/comments/{id}

class Comment extends Spine.Model
  @configure "Comment", "photo_id", "other", "stuff"
  @extend Spine.Model.Ajax

  scope: -> "/photos/#{@photo_id}"

Or perhaps somebody wants to use the same spine model for both normal /models/{id} and admin /admin/models/{id} api. This could provide a fairly easy way to do it..

scope: -> if @admin then "/admin" else ""

any thoughts/opinions? :o)

@aeischeid
Copy link
Member

My first thought on instance score assignment is that it would be an odd api design to require resources with different flags to use different paths, but then I realize that we have a couple odd cases like that. For example a resource moves to a state where it is not normally able to be updated, but we have one property we still want to be able to update so we made special API path for that one task... maybe that is bad API design... it our case simply added a method for sending off to that path rather than using a simple save, not a big deal. Wow, all that to say spine should probably encourage good API design, rather than catering to bad ones.

The change as a way to define scope for related models seems like a pretty good idea. The 'take it a step further' per instance redefinition idea seem like a less good one, but I am still open to it.

@Nemesisprime
Copy link

I think this relates directly with my ticket, I actually ended up using a hack to get around this problem. I'd be glad to see some sort of model scope functionality this functionality built in.

@cengebretson
Copy link
Member

So right now, the way you guys are handling this is overriding the @url property on the Model? Something like this?

@url: -> "/project" + super

@sishen
Copy link
Contributor Author

sishen commented Feb 16, 2013

Indeed, I have two kinds of scopes in my own fork, one is Collection based and another is Instance based. For example,

  @scope: ->
    "/projects/#{current.project_id}"

  scope: ->
    "/projects/#{@project_id}"

However, I found that all my cases I can just use the Collection based scope. And what's more, in order to use the Instance based scope, I have to duplicate the url generation instead of reusing the part in Extend part.

Include =
  ajax: -> new Singleton(this)

  url: (args...) ->
    args.unshift(encodeURIComponent(@id))
    args.unshift(@className.toLowerCase() + 's')
    scope = this.scope?() or this.scope
    if scope?
      scope = scope.substring(1) if scope.charAt(0) is '/'
      args.unshift(scope)
    args.unshift(Model.host)
    args.join('/')

So I'm not sure whether it's a good idea to have two different methods. Thoughts?

@cengebretson
Copy link
Member

It does seem that overriding the url() function would be the natural place for this sort of thing but like you mention the logic is split between the single instance and the collection version. Even when dealing with a model instance the ajax calls are split between the two. When performing a GET, DELETE, or PUT the instance url() is called, while a POST will call the url() method on the class/model.

Maybe there is a way to tweak the url() methods so most of the logic is placed in the Collection version. If we can change the way urls are constructed so that there was only method to override, would that help with things like creating scope or perhaps adding some other dynamic path to the final url?

@sishen
Copy link
Contributor Author

sishen commented Feb 19, 2013

@cengebretson The current implementation is similar like what you suggest as the url() method in instance only append the resource id. The problem is that if the instance scope is different than model scope, we still need to define two scope or other dynamic path even though it's expected. However, we can pass the scope parameter in the instance url() to let the Collection version handle most of the logic. For example,

Include =
  ajax: -> new Singleton(this)

  url: (args...) ->
    url = Ajax.getURL(@constructor, @scope?() or @scope)
    url += '/' unless url.charAt(url.length - 1) is '/'
    url += encodeURIComponent(@id)
    args.unshift(url)
    args.join('/')

I have one version of such patch. It's not elegant but works.

@aeischeid
Copy link
Member

Seems like it might be worth investing in the underlying implementation a bit.

@sishen
Copy link
Contributor Author

sishen commented Feb 20, 2013

@aeischeid Yup. Any suggestions? I can work on it and update the PR. Thanks.

@aeischeid
Copy link
Member

Sorry, I don't have anything insightful on how exactly to go about the refactoring of the underlying url bits. Feels like this discussion has nailed down the problem a little better, so at least you've got that to start from. ;)

The rules to apply the scopes are:

1. If instance scope defined, the instance url will apply that scope.
2. If instance scope not defined, the instance url will apply the model
   scope if there is.
3. The model url will only apply the model scope if defined.
@sishen
Copy link
Contributor Author

sishen commented Feb 26, 2013

Scope can apply to collection/model level and object/instance level.

The rules to apply the scope are:

  1. If instance scope defined, the instance url will apply that scope.
  2. If instance scope not defined, the instance url will apply the model scope if there is.
  3. The model url will only apply the model scope if defined.

@aeischeid
Copy link
Member

Appreciate your work on this. Just to keep you updated we want to get this feature in but just haven't gotten around to it yet.

@sishen
Copy link
Contributor Author

sishen commented Mar 8, 2013

Thanks for the update.

aeischeid added a commit that referenced this pull request Apr 12, 2013
@aeischeid aeischeid closed this Apr 12, 2013
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.

None yet

5 participants