This repository was archived by the owner on Apr 12, 2024. It is now read-only.
refactor($compile): remove unnecessary and invalid $destroyed listener #12528
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| for (var i = 0, ii = onNewScopeDestroyed.length; i < ii; ++i) { | ||
| onNewScopeDestroyed[i](); | ||
| } | ||
| } : noop; | ||
| if (newScope && destroyBindings !== noop) { | ||
| newScope.$on('$destroy', destroyBindings); |
There was a problem hiding this comment.
I moved this out to the 1 of 3 initializeDirectiveBindings invocations that actually need it. I figured now that initializeDirectiveBindings binds to any destination and not specifically a scope this logic no longer belonged here...
|
@petebacondarwin WDYT? |
| initializeDirectiveBindings(scope, attrs, isolateScope, | ||
| isolateScope.$$isolateBindings, | ||
| newIsolateScopeDirective, isolateScope); | ||
| var parentWatchDestoryer = initializeDirectiveBindings(scope, attrs, isolateScope, |
There was a problem hiding this comment.
-> Destroyer (also on line 1984 and 1985)
|
I think the commit message should be more explicit: why exactly do we not need to listen to $destroy in this case? |
jbedard
force-pushed
the
compile-bindings-destroy-listener
branch
from
9f210a5 to
cf6087b
Compare
|
Fixed the var name typo and rebased. I'll have to look into it again to remember why the listener isn't actually needed... |
petebacondarwin
added a commit
to petebacondarwin/angular.js
that referenced
this pull request
Oct 7, 2015
petebacondarwin
added a commit
to petebacondarwin/angular.js
that referenced
this pull request
Oct 7, 2015
This check means that we don't have to keep checking whether the collection has been created when adding a new watcher Closes angular#12528
petebacondarwin
added a commit
to petebacondarwin/angular.js
that referenced
this pull request
Oct 7, 2015
Previously we assigned `noop` if there was no function but there is no performance advantage in doing this since the check would have to happen either at assignment time or at call time. Removing this use of `noop` makes the code clearer, IMO :-) Closes angular#12528
petebacondarwin
pushed a commit
that referenced
this pull request
Oct 7, 2015
…lizeDirectiveBindings Since only one of three invocations of `initializeDirectiveBindings` actually adds a `$destroy` handler to the scope (the others just manually call unwatch as needed), we can move that code out of this method. This also has the benefit of simplifying what parameters need to be passed through to the linking functions Closes #12528
petebacondarwin
added a commit
that referenced
this pull request
Oct 7, 2015
petebacondarwin
added a commit
that referenced
this pull request
Oct 7, 2015
This check means that we don't have to keep checking whether the collection has been created when adding a new watcher Closes #12528
petebacondarwin
added a commit
that referenced
this pull request
Oct 7, 2015
Previously we assigned `noop` if there was no function but there is no performance advantage in doing this since the check would have to happen either at assignment time or at call time. Removing this use of `noop` makes the code clearer, IMO :-) Closes #12528
|
I tweaked, embellished and merged. Thanks @jbedard - do you just spend your evenings scouring the compiler?? |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I noticed when bindToController is used the scope
$destroyedevent is listened for, but it should be$destroy. But looking further into it I don't think we need to listen for any destroy event in this case so this PR just deletes it.