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

Always delegate in #setElement. #3060

Merged
merged 1 commit into from
Mar 13, 2014
Merged

Always delegate in #setElement. #3060

merged 1 commit into from
Mar 13, 2014

Conversation

braddunbar
Copy link
Collaborator

Ok, crazy question. Why do we delegate events after calling View#initialize instead of before? The test suite passes either way so I have only conjecture. Here are a couple:

  1. Changing el in #initialize - This is probably ill advised but changing this doesn't alter behavior. Events would just be delegated twice.
  2. Altering #events in #initialize - I suppose you could, but I would say this is also ill advised. Regardless, you could do it in the constructor if you had to.

Anything I missed? This allows us to do away with the delegate argument to setElement, which is a clear win I think.

@wyuenho
Copy link
Contributor

wyuenho commented Mar 13, 2014

Another question to ask would be, why would anyone want to not redelegate events when setting a new el?

@akre54
Copy link
Collaborator

akre54 commented Mar 13, 2014

👍

@jashkenas
Copy link
Owner

I think because you might want to go and trigger an event during initialize...

But if you think it's clearly better —

@braddunbar
Copy link
Collaborator Author

Hmm...you mean you might want to trigger an event before attaching listeners? I don't quite follow.

@jashkenas
Copy link
Owner

Sorry — you're right. I was confused.

jashkenas added a commit that referenced this pull request Mar 13, 2014
@jashkenas jashkenas merged commit b5f1574 into jashkenas:master Mar 13, 2014
@braddunbar braddunbar deleted the delegate-events branch March 13, 2014 22:38
paulfalgout added a commit to paulfalgout/backbone.marionette that referenced this pull request Jul 3, 2014
This may be very minor or even a non-issue... certainly less of an issue until the next backbone release.

Backbone.View.delegateEvents is for explicitly setting up events on the.$el and this is even more evident with this PR for an upcoming release:  jashkenas/backbone#3060

Marionette Entity events don't need to be re-setup if the $el changes.

Really it seems they may need only be bound in the constructor and cleaned up, as they will be anyways, on destroy.  Then if you needed to dynamically change collectionEvents or modelEvents for some reason, you'd use Marionette.View.bindEntityEvents

However a change like this would be breaking for anyone currently using delegateEvents as a reset for if changes are made to modelEvents or collectionEvents.
akre54 added a commit to akre54/backbone that referenced this pull request May 27, 2015
jashkenas added a commit that referenced this pull request May 27, 2015
akre54 added a commit that referenced this pull request May 27, 2015
@jridgewell
Copy link
Collaborator

Might be worth reconsidering this one, there are now 6 bug reports (#3166, #3256, #3704, #3620, marionettejs/backbone.marionette#2605, marionettejs/backbone.marionette#2628) that I know of from users about modifying the events hash in #initialize.

@akre54
Copy link
Collaborator

akre54 commented Jul 7, 2015

This seems like something that you should've been doing in the first place. Not sure why we'd roll it back.

@jridgewell
Copy link
Collaborator

Just using libraries I've written on top of Backbone as an example, I always do binding after calling #initialize. In fact, I've recommended it.

Yes, it can be solved by using a function instead, but Backbone's encouraged extend using objects doesn't make it obvious. Modifying in an #initialize is much easier for end devs to grasp.

@akre54
Copy link
Collaborator

akre54 commented Jul 7, 2015

But why? initialize isn't the right place for this, in my opinion. If your events aren't ready to be bound until after you've rendered, just put a delegate call at the end of your render (or set it up in your child view). Setting up event listeners and child views in initialize is fine, but I wouldn't mess with your el or child views' el there.

@jridgewell
Copy link
Collaborator

But why? initialize isn't the right place for this, in my opinion.

I think that's fighting against a common usage. It's not just new devs that are falling into it, I do it as well.

If your events aren't ready to be bound until after you've rendered, just put a delegate call at the end of your render (or set it up in your child view).

But these aren't waiting for a render to bind new events handlers. It's about treating #initialize as a proper constructor:

The thing is initialize exists so we dont have to override the constructor and call super. This same existence can be reconsidered as soon as we have to move things into constructor again, just to make things work. — marionettejs/backbone.marionette#2605 (comment)

We've encouraged this usage pattern by providing an #initialize.


As a side note, library authors aren't able to provide this same functionality. You can either bind this like modelEvents before calling Backbone.View.apply(this, arguments) in which case you won't have any of the viewOptions on the instance, or afterwards in which case #initialize has already run.

@kworth
Copy link

kworth commented Sep 23, 2015

Doesn't #3802 bring up a new point to this discussion? It appears we've discussed modifying el in initialize and things like that. So I won't beat that horse. But what about the pattern of assigning a model in the initialize, etc. mentioned in #3802?

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.

6 participants