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

Make model parameter optional for Backbone.sync #4002

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

creynders
Copy link

I encountered a situation in which I needed to call Backbone.sync directly, w/o a model, which works perfectly except it will throw when trying to trigger a 'request' event on model. Obviously I could easily pass a trigger:noop object or something, but thought this was cleaner.
Test included btw.

@akre54
Copy link
Collaborator

akre54 commented Mar 31, 2016

Sorry but -- what's the use case? Every extra conditional is one more potential bug.

@creynders
Copy link
Author

It's a bit hard to explain, but basically it comes down to this: I have a number of things that are automatically attached to all Backbone.sync requests (e.g. a unique ID for each request, centralised error handling et cetera), which is why I'd like to use Backbone.sync for a multipart/formdata RPC request, to benefit from the extras, but all the data it's sending is massaged and collected, i.e. there's no specific model that I need to use. As I said, I can easily solve it, but I assumed that model was in fact meant to be optional since on this line it's truthiness is being checked as well, i.e. why bother checking if it's required anyway?
If you dig through the function you'll see there's no real dependency on model except to use as data if none is provided and to send out a 'request' notification, which struck me as it being optional. But no problem if it's too much a hassle.

@akre54
Copy link
Collaborator

akre54 commented Mar 31, 2016

To be honest, I'm not sure why that's there (but it's been there for years), along with the model.trigger. I think wrapping sync with a dummy object is going to be your best bet here, as yours is probably not a use case we want to be supporting. I'll leave this issue open a little longer in case any other contributors want to chime in.

@akre54 akre54 added the change label Mar 31, 2016
@jridgewell
Copy link
Collaborator

We should either add this, or remove the&& model guard.

@Florian-R
Copy link

I was curious about this guard and for posterity it was introduced by 4c1bdb4. Seems safe to remove it.

@creynders
Copy link
Author

Back then the guard made totally sense, since model was optional... Which I think TBH it still should be 😁

@akre54
Copy link
Collaborator

akre54 commented Apr 1, 2016

or remove the && model guard.

I thought of that (removing it doesn't fail any tests), but what's the harm in keeping it?

In this case it's probably fine to remove, but I think recent history has shown that we tend to get bit by edge cases we think are safe to get rid of. Let's just make sure we have tests covering the expected behaviors.

@megawac megawac mentioned this pull request May 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants