Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

feat(shutdown): Add the ability for an app to shutdown #8005

Closed
wants to merge 1 commit into from
Closed

feat(shutdown): Add the ability for an app to shutdown #8005

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/.jshintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@
"angularInit": false,
"bootstrap": false,
"getTestability": false,
"shutdown": false,
"snake_case": false,
"bindJQuery": false,
"assertArg": false,
Expand Down
44 changes: 43 additions & 1 deletion src/Angular.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1692,7 +1692,7 @@ function bootstrap(element, modules, config) {

modules = modules || [];
modules.unshift(['$provide', function($provide) {
$provide.value('$rootElement', element);
$provide.provider('$rootElement', rootElementProviderFactory(element));
}]);

if (config.debugInfoEnabled) {
Expand Down Expand Up @@ -1772,6 +1772,48 @@ function getTestability(rootElement) {
return injector.get('$$testability');
}

function rootElementProviderFactory(rootElement) {
return ['$shutdownProvider', function($shutdownProvider) {
$shutdownProvider.register(function() {
if (rootElement.dealoc) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under what circumstances can rootElement.dealoc be defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, this is just proof of how old this PR is, this was removed with 9c5b407

WDYT about changing this to

jqLite(rootElement[0].querySelectorAll('*')).removeData();
rootElement.removeData();

?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically the same as the else branch below, so we only need one branch.

rootElement.dealoc();
} else {
rootElement.find('*').removeData();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a common case for sure, but this would also "clean" inside [ng-non-bindable] element (where theoretically could leave other angular apps).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there is an outer and an inner app, and the outer app is shutdown (remember that this will trigger a $destroy event on $rootScope), I would expect that it is reasonable for the inner app to also get destroyed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it shouldn't. These are independent apps, so destroying the outer, shouldn't destroy the inner. To be clear, this is what I am talking about:

<body app1>
  Hello from {{ 'app 1' }}!
  <div ng-non-bindable>
    <div app2>
      Hello from {{ 'app 2' }}!
    </div>
  </div>
</body>
angular.module('app1', []);
angular.module('app2', []);

angular.bootstrap(document.querySelector('[app1]'), ['app1']);
angular.bootstrap(document.querySelector('[app2]'), ['app2']);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this is currently not possible, as in cases like this we do not keep track of what elements belong to app1 and what elements belong to app2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible to ignore anything under ng-non-bindable (because we haven't touched it anyway).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like this (which is not super efficient, but shouldn't really matter, when shutting down):

var unvisitedNodes = [$rootElement[0]];
var foundNodes = [$rootElement[0]];
var node;

while ((node = unvisitedNodes.pop())) {
  var newNodes = Array.prototype.filter.call(node.children, function (child) {
    return directiveNormalize(nodeName_(child)) !== 'ngNonBindable';
  });
  Array.prototype.push.apply(unvisitedNodes, newNodes);
  Array.prototype.push.apply(foundNodes, newNodes);
}

jqLite.cleanData(foundNodes);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gkalpak if we want to support nested ng-apps in ng-non-bindable containers perhaps we should add at least basic tests verifying that it works?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps. But basic tests wouldn't help much (I mean even if basic tests worked, some less basic code could still be broken). The question is whether we want to break this usecase or not (I still wouldn't).

rootElement.removeData();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not enough as it won't remove event listeners. How about:

jqLite.cleanData(rootElement.find('*'));
jqLite.cleanData(rootElement);

or something analogous to @gkalpak's code but using cleanData if we want to not break ng-non-bindable-contained AngularJS apps.

This, BTW, made me notice that jqLite's implementation of removeData is really broken. It should only remove user-set data and not touch the event handlers storage at all. I reported #15869 to fix that in 1.7.

}
});
this.$get = function() {
return rootElement;
};
}];
}

function $ShutdownProvider() {
var fns = [];
this.$get = function() {
return function() {
while (fns.length) {
var fn = fns.shift();
fn();
}
};
};

this.register = function(fn) {
fns.push(fn);
};
}

function shutdown(element) {
var injector;

injector = angular.element(element).injector();
if (!injector) {
throw ngMinErr('shtdwn', 'Element not part of an app');
}
injector.get('$shutdown')();
}

var SNAKE_CASE_REGEXP = /[A-Z]/g;
function snake_case(name, separator) {
separator = separator || '_';
Expand Down
4 changes: 4 additions & 0 deletions src/AngularPublic.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@
$$SanitizeUriProvider,
$SceProvider,
$SceDelegateProvider,
$ShutdownProvider,
$SnifferProvider,
$TemplateCacheProvider,
$TemplateRequestProvider,
Expand Down Expand Up @@ -125,6 +126,7 @@ var version = {
function publishExternalAPI(angular) {
extend(angular, {
'bootstrap': bootstrap,
'shutdown': shutdown,
'copy': copy,
'extend': extend,
'merge': merge,
Expand Down Expand Up @@ -160,6 +162,8 @@ function publishExternalAPI(angular) {

angularModule('ng', ['ngLocale'], ['$provide',
function ngModule($provide) {
// $shutdown provider needs to be first as other providers might use it.
$provide.provider('$shutdown', $ShutdownProvider);
// $$sanitizeUriProvider needs to be before $compileProvider as it is used by it.
$provide.provider({
$$sanitizeUri: $$SanitizeUriProvider
Expand Down
95 changes: 76 additions & 19 deletions src/ng/browser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,11 @@ function Browser(window, document, $log, $sniffer) {
history = window.history,
setTimeout = window.setTimeout,
clearTimeout = window.clearTimeout,
pendingDeferIds = {};
setInterval = window.setInterval,
clearInterval = window.clearInterval,
pendingDeferIds = {},
currentIntervalIds = {},
active = true;

self.isMock = false;

Expand Down Expand Up @@ -79,6 +83,19 @@ function Browser(window, document, $log, $sniffer) {
}
};

self.shutdown = function() {
active = false;
forEach(currentIntervalIds, function(ignore, intervalId) {
delete currentIntervalIds[intervalId];
clearInterval(+intervalId);
});
forEach(pendingDeferIds, function(ignore, timeoutId) {
delete pendingDeferIds[timeoutId];
clearTimeout(+timeoutId);
});
jqLite(window).off('hashchange popstate', cacheStateAndFireUrlChange);
};
Copy link
Member

@gkalpak gkalpak Jun 23, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it wasn't done before either, but wouldn't it make sense to clear timeouts as well?


//////////////////////////////////////////////////////////////
// URL API
//////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -270,16 +287,6 @@ function Browser(window, document, $log, $sniffer) {
return callback;
};

/**
* @private
* Remove popstate and hashchange handler from window.
*
* NOTE: this api is intended for use only by $rootScope.
*/
self.$$applicationDestroyed = function() {
jqLite(window).off('hashchange popstate', cacheStateAndFireUrlChange);
};

/**
* Checks whether the url has changed outside of Angular.
* Needs to be exported to be able to check for changes that have been done in sync,
Expand Down Expand Up @@ -321,12 +328,16 @@ function Browser(window, document, $log, $sniffer) {
*/
self.defer = function(fn, delay) {
var timeoutId;
outstandingRequestCount++;
timeoutId = setTimeout(function() {
delete pendingDeferIds[timeoutId];
completeOutstandingRequest(fn);
}, delay || 0);
pendingDeferIds[timeoutId] = true;
if (active) {
outstandingRequestCount++;
timeoutId = setTimeout(function() {
delete pendingDeferIds[timeoutId];
completeOutstandingRequest(fn);
}, delay || 0);
pendingDeferIds[timeoutId] = true;
} else {
timeoutId = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of this line?

}
return timeoutId;
};

Expand All @@ -351,11 +362,57 @@ function Browser(window, document, $log, $sniffer) {
return false;
};


/**
* @name $browser#interval
* @param {function()} fn A function, whose execution should be executed.
* @param {number=} interval Interval in milliseconds on how often to execute the function.
* @returns {*} IntervalId that can be used to cancel the task via `$browser.interval.cancel()`.
*
* @description
* Executes a fn asynchronously via `setInterval(fn, interval)`.
*
*/
self.interval = function(fn, interval) {
var intervalId;
if (active) {
intervalId = setInterval(fn, interval);
currentIntervalIds[intervalId] = true;
} else {
intervalId = 0;
}
return intervalId;
};


/**
* @name $browser#interval.cancel
*
* @description
* Cancels an interval task identified with `intervalId`.
*
* @param {*} intervalId Token returned by the `$browser.interval` function.
* @returns {boolean} Returns `true` if the task was successfully canceled, and
* `false` if the task was already canceled.
*/
self.interval.cancel = function(intervalId) {
if (currentIntervalIds[intervalId]) {
delete currentIntervalIds[intervalId];
clearInterval(intervalId);
return true;
}
return false;
};
}

function $BrowserProvider() {
function $BrowserProvider($shutdownProvider) {
var browser;

$shutdownProvider.register(function() { if (browser) { browser.shutdown(); } });
this.$get = ['$window', '$log', '$sniffer', '$document',
function($window, $log, $sniffer, $document) {
return new Browser($window, $document, $log, $sniffer);
return browser = new Browser($window, $document, $log, $sniffer);
}];
}

$BrowserProvider.$inject = ['$shutdownProvider'];
8 changes: 3 additions & 5 deletions src/ng/interval.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,16 +135,14 @@ function $IntervalProvider() {
function interval(fn, delay, count, invokeApply) {
var hasParams = arguments.length > 4,
args = hasParams ? sliceArgs(arguments, 4) : [],
setInterval = $window.setInterval,
clearInterval = $window.clearInterval,
iteration = 0,
skipApply = (isDefined(invokeApply) && !invokeApply),
deferred = (skipApply ? $$q : $q).defer(),
promise = deferred.promise;

count = isDefined(count) ? count : 0;

promise.$$intervalId = setInterval(function tick() {
promise.$$intervalId = $browser.interval(function tick() {
if (skipApply) {
$browser.defer(callback);
} else {
Expand All @@ -154,7 +152,7 @@ function $IntervalProvider() {

if (count > 0 && iteration >= count) {
deferred.resolve(iteration);
clearInterval(promise.$$intervalId);
$browser.interval.cancel(promise.$$intervalId);
delete intervals[promise.$$intervalId];
}

Expand Down Expand Up @@ -191,7 +189,7 @@ function $IntervalProvider() {
// Interval cancels should not report as unhandled promise.
intervals[promise.$$intervalId].promise.catch(noop);
intervals[promise.$$intervalId].reject('canceled');
$window.clearInterval(promise.$$intervalId);
$browser.interval.cancel(promise.$$intervalId);
delete intervals[promise.$$intervalId];
return true;
}
Expand Down
17 changes: 11 additions & 6 deletions src/ng/rootScope.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,13 @@
* They also provide event emission/broadcast and subscription facility. See the
* {@link guide/scope developer guide on scopes}.
*/
function $RootScopeProvider() {
$RootScopeProvider.$inject = ['$shutdownProvider'];
function $RootScopeProvider($shutdownProvider) {
var TTL = 10;
var $rootScopeMinErr = minErr('$rootScope');
var lastDirtyWatch = null;
var applyAsyncId = null;
var $rootScope;

this.digestTtl = function(value) {
if (arguments.length) {
Expand All @@ -94,8 +96,10 @@ function $RootScopeProvider() {
return ChildScope;
}

this.$get = ['$exceptionHandler', '$parse', '$browser',
function($exceptionHandler, $parse, $browser) {
$shutdownProvider.register(function() { if ($rootScope) { $rootScope.$destroy(); } });

this.$get = ['$exceptionHandler', '$parse', '$browser', '$shutdown',
function($exceptionHandler, $parse, $browser, $shutdown) {

function destroyChildScope($event) {
$event.currentScope.$$destroyed = true;
Expand Down Expand Up @@ -907,8 +911,7 @@ function $RootScopeProvider() {
this.$$destroyed = true;

if (this === $rootScope) {
//Remove handlers attached to window when $rootScope is removed
$browser.$$applicationDestroyed();
$shutdown();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure we need this, now that $shutdown is a public service.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gkalpak What do you mean?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's been a while, but what I think I meant that this was a poor man's $shutdown(). Before $shutdown() was implemented, we were special-casing $rootScope.$destroy() to do destroy the application (because $browser.$$applicationDestroyed() was private). But now that $shutdown() is a public service, people wanting to destroy the app can (and should) use that (instead of destroying $rootScope.

It doesn't seem too harmful though, so we might as well keep it to avoid the breaking change.

}

incrementWatchersCount(this, -this.$$watchersCount);
Expand Down Expand Up @@ -1308,7 +1311,7 @@ function $RootScopeProvider() {
}
};

var $rootScope = new Scope();
$rootScope = new Scope();

//The internal queues. Expose them on the $rootScope for debugging/testing purposes.
var asyncQueue = $rootScope.$$asyncQueue = [];
Expand Down Expand Up @@ -1374,3 +1377,5 @@ function $RootScopeProvider() {
}
}];
}


Loading