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.

Sign up for GitHub

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

Jump to bottom

[BUGFIX lts] More assertions for Application lifecycle methods #18960

Merged
merged 1 commit into from
May 9, 2020
Merged

[BUGFIX lts] More assertions for Application lifecycle methods #18960

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
70 changes: 64 additions & 6 deletions packages/@ember/application/lib/application.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,16 @@ const Application = Engine.extend({
@return {ApplicationInstance} the application instance
*/
buildInstance(options = {}) {
assert(
'You cannot build new instances of this application since it has already been destroyed',
!this.isDestroyed
);

assert(
'You cannot build new instances of this application since it is being destroyed',
!this.isDestroying
);

options.base = this;
options.application = this;
return ApplicationInstance.create(options);
Expand Down Expand Up @@ -516,7 +526,7 @@ const Application = Engine.extend({
@method domReady
*/
domReady() {
if (this.isDestroyed) {
if (this.isDestroying || this.isDestroyed) {
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, once we go this.isDestroying = true we never change it again. I think checking both makes the code easier to read (and avoids everyone having to internalize that particular detail) but I don't think it is actually going to change anything about the semantics.

return;
}

Expand Down Expand Up @@ -560,10 +570,19 @@ const Application = Engine.extend({
'You must call deferReadiness on an instance of Application',
this instanceof Application
);

assert('You cannot defer readiness since application has already destroyed', !this.isDestroyed);

assert(
'You cannot defer readiness since the application is being destroyed',
!this.isDestroying
);

assert(
'You cannot defer readiness since the `ready()` hook has already been called.',
'You cannot defer readiness since the `ready()` hook has already been called',
this._readinessDeferrals > 0
);

this._readinessDeferrals++;
},

Expand All @@ -581,6 +600,22 @@ const Application = Engine.extend({
'You must call advanceReadiness on an instance of Application',
this instanceof Application
);

assert(
'You cannot advance readiness since application has already destroyed',
!this.isDestroyed
);

assert(
'You cannot advance readiness since the application is being destroyed',
!this.isDestroying
);

assert(
'You cannot advance readiness since the `ready()` hook has already been called',
this._readinessDeferrals > 0
);

this._readinessDeferrals--;

if (this._readinessDeferrals === 0) {
Expand All @@ -605,6 +640,13 @@ const Application = Engine.extend({
@return {Promise<Application,Error>}
*/
boot() {
assert(
'You cannot boot this application since it has already been destroyed',
!this.isDestroyed
);

assert('You cannot boot this application since it is being destroyed', !this.isDestroying);

if (this._bootPromise) {
return this._bootPromise;
}
Expand Down Expand Up @@ -633,7 +675,7 @@ const Application = Engine.extend({
@private
*/
_bootSync() {
if (this._booted) {
if (this._booted || this.isDestroying || this.isDestroyed) {
return;
}

Expand Down Expand Up @@ -730,6 +772,13 @@ const Application = Engine.extend({
@public
*/
reset() {
assert(
'You cannot reset this application since it has already been destroyed',
!this.isDestroyed
);

assert('You cannot reset this application since it is being destroyed', !this.isDestroying);

assert(
`Calling reset() on instances of \`Application\` is not
supported when globals mode is disabled; call \`visit()\` to
Expand Down Expand Up @@ -759,6 +808,10 @@ const Application = Engine.extend({
@method didBecomeReady
*/
didBecomeReady() {
if (this.isDestroying || this.isDestroyed) {
return;
}

try {
// TODO: Is this still needed for _globalsMode = false?
if (!isTesting()) {
Expand Down Expand Up @@ -819,10 +872,8 @@ const Application = Engine.extend({
// This method must be moved to the application instance object
willDestroy() {
this._super(...arguments);

setNamespaceSearchDisabled(false);
this._booted = false;
this._bootPromise = null;
this._bootResolver = null;

if (_loaded.application === this) {
_loaded.application = undefined;
Expand Down Expand Up @@ -1032,6 +1083,13 @@ const Application = Engine.extend({
@return {Promise<ApplicationInstance, Error>}
*/
visit(url, options) {
assert(
'You cannot visit this application since it has already been destroyed',
!this.isDestroyed
);

assert('You cannot visit this application since it is being destroyed', !this.isDestroying);

return this.boot().then(() => {
let instance = this.buildInstance();

Expand Down