search

AmpersandJS / ampersand-view

A smart base view for Backbone apps, to make it easy to bind collections and properties to the DOM.
http://ampersandjs.com
MIT License
92 stars 39 forks source link

Run _upsertBindings before custom render function #181

Closed herkyl closed 6 years ago

herkyl commented 8 years ago

Previously when you remove()'d a subview and then rendered it again (for instance using renderSubview()) the bindings broke. This happens because remove() calls _downsertBindings() but a custom render() method did not call _upsertBindings().

This pull request makes a custom render() function work like the default one and calls _upsertBindings().

herkyl commented 8 years ago

How embarrassing.. :P. I fixed it

wraithgar commented 8 years ago

Looks good, I wonder what a test of this would look like?

herkyl commented 8 years ago

I will add a test when I get some time

RickButler commented 7 years ago

I thought not running _upsertBindings in custom render functions was intentional for backwards compatibility. In a lot of my projects, I run the view's prototype render inside my custom render function.

This will make it hard to have a render that does not call _upsertBindings. If I had a view that I didn't want to run _upsertBindings in the render (I have seen people put a fetch inside render, and on fetch success then they pass it into the template and set the el). So in order to remove that _upsertBindings call, I would have to make changes to _setRender which is a private method, and would most likely cause breaking changes at some point.

I'm not opposed to this change (just pointing out things we may not have considered). If this the direction we want to go, I would also point out that we should make the corresponding change for _setRemove to call _downsertBindings.

ruiramos commented 7 years ago

+1 for calling _downsertBindings on the _remove setter and merging this in.

RickButler commented 6 years ago

Didn't realize I could push changes to @herkyl branch.

Rebased, calling _downsertBindings in_setRemove, and added some tests.

Because people may intentionally not being calling upsert/downsert in their render/removes, this should probably be classified as breaking. So SemVer major bump unless someone convinces me not to.

Tests are simple, just looking at bindingsSet to determine if upsert/downsert was called. I didn't add a test to verifying custom render/remove function, because that is covered in: https://github.com/AmpersandJS/ampersand-view/blob/f63202190fcf670022dfacf19830c1efef847098/test/main.js#L43

Looking for a +1 before I merge this, as I added some work myself.

dhritzkiv commented 6 years ago

I'll let you merge it, and create a new release on npm, @RickButler.