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

Align few places to match with possible overriding of Backbone.sync #3582

Closed
wants to merge 1 commit into from

Conversation

Artazor
Copy link

@Artazor Artazor commented Apr 28, 2015

Overriding Backbone.sync is a great way to switch a persistence layer. It just lacks two moments that should be overridable too:

  • Result of model.save if model was not validated
  • Result of model.destory if model was new

For example if my implementation of Backbone.sync returns promises, then current implementation forces me to check the result before calling '.then()'. With the proposed changes, we will be able to unify sync behavior with the rest of the Backbone.

@jashkenas ?

…e.sync

Overriding Backbone.sync is a great way to switch a persistence layer. It just lacks two moments that should be overridable too:

* Result of `model.save` if model was not validated
* Result of `model.destory` if model was new

For example if my implementation of Backbone.sync returns promises, then current implementation forces me to check the result before calling '.then()'. With the proposed changes, we will be able to unify `sync` behavior with the rest of the Backbone.

@jashkenas ?
@jridgewell
Copy link
Collaborator

I think this would be more useful if we always returned a thenable?

@Artazor
Copy link
Author

Artazor commented Apr 28, 2015

@jridgewell Yes!!! It would be awesome, but it will be backward incompatible (I suppose).

Anyway there will be a leak of thenable in Collection::create() as long as it uses thenable-returning operation Model::save() but does not return created thenable (it returns a model)

@Artazor
Copy link
Author

Artazor commented Apr 28, 2015

Anyway this PR will help me not to maintain my own version of Backbone -)
Would appreciate any ideologically compatible change/merge

@akre54
Copy link
Collaborator

akre54 commented Apr 28, 2015

I think this would be more useful if we always returned a thenable?

Time to revisit #2489?

@Artazor
Copy link
Author

Artazor commented Apr 28, 2015

@akre54 Yes its just about the same issue. However I think that the current PR is way more tiny, yet sufficient to use watever promise library you want.

However Backbone can implement it's own thenable stubs since in both mentioned cases returned promises are already settled (i.e. have very limited range of reactions for .then())

@jridgewell
Copy link
Collaborator

Time to revisit #2489?

👍

@jashkenas
Copy link
Owner

Let's go revisit over there then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants