This repository has been archived by the owner on May 15, 2019. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 99
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
Joel Burget
committed
Jun 29, 2014
1 parent
1c31a8c
commit 6434712
Showing
3 changed files
with
111 additions
and
1 deletion.
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
var $ = require("jquery"); | ||
|
||
var UNMOUNTING = {}; | ||
|
||
/* If the component unmounts before the promise is resolved: | ||
* ~ the fullfill handler is not called | ||
* ~ the reject handler is called with PromiseMixin.UNMOUNTING | ||
* ~ we make no effort to cancel the promise | ||
*/ | ||
|
||
var PromiseMixin = { | ||
defer: function(promise) { | ||
var ret = $.Deferred(); | ||
this._deferreds.push(ret); | ||
|
||
promise.then( | ||
function() { ret.resolveWith(this, [].slice.call(arguments)); }, | ||
function() { ret.rejectWith(this, [].slice.call(arguments)); } | ||
); | ||
|
||
return ret.promise(); | ||
}, | ||
|
||
componentWillUnmount: function() { | ||
this._deferreds.map(deferred => deferred.reject(UNMOUNTING)); | ||
|
||
// TODO necessary? | ||
delete this._deferreds; | ||
}, | ||
|
||
_deferreds: [], | ||
|
||
UNMOUNTING | ||
}; | ||
|
||
module.exports = PromiseMixin; |
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
/** @jsx React.DOM */ | ||
|
||
var jsdom = require("jsdom"); | ||
var assert = require("assert"); | ||
var React = require("react/addons"); | ||
var TestUtils = React.addons.TestUtils; | ||
var PromiseMixin = require("../js/deferred.jsx"); | ||
var $ = require("jquery"); | ||
|
||
describe("PromiseMixin", function() { | ||
beforeEach(function() { | ||
global.window = jsdom.jsdom().createWindow(); | ||
global.document = window.document; | ||
|
||
var Class = React.createClass({ | ||
mixins: [PromiseMixin], | ||
render: function() { | ||
return <div />; | ||
} | ||
}); | ||
|
||
this.node = document.createElement('div'); | ||
this.component = React.renderComponent(<Class />, this.node); | ||
}); | ||
|
||
it("rejects when unmounting", function() { | ||
var deferred = $.Deferred(); | ||
var rejected = false; | ||
var resolved = false; | ||
this.component.defer(deferred).then( | ||
() => { resolved = true; }, | ||
() => { rejected = true; } | ||
); | ||
React.unmountComponentAtNode(this.node); | ||
|
||
assert.strictEqual(resolved, false); | ||
assert.strictEqual(rejected, true); | ||
}); | ||
|
||
it("passes through a resolution", function() { | ||
var deferred = $.Deferred(); | ||
var result = null; | ||
this.component.defer(deferred).then(r => { result = r; }); | ||
|
||
var unique = {}; | ||
deferred.resolve(unique); | ||
|
||
assert.strictEqual(result, unique); | ||
}); | ||
|
||
it("passes through a rejection", function() { | ||
var deferred = $.Deferred(); | ||
var result = null; | ||
this.component.defer(deferred).then(null, r => { result = r; }); | ||
|
||
var unique = {}; | ||
deferred.reject(unique); | ||
|
||
assert.strictEqual(result, unique); | ||
}); | ||
|
||
// testing the raison d'etre of this mixin - we just need it to not throw | ||
// an exception | ||
it("doesn't barf when you resolve after unmounting", function() { | ||
var deferred = $.Deferred(); | ||
this.component.defer(deferred).then( | ||
() => { this.setState({ blowUp: true }); }, | ||
null | ||
); | ||
React.unmountComponentAtNode(this.node); | ||
deferred.resolve(true); | ||
}); | ||
}); |
6434712
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmnd feedback?
6434712
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if the small amount of sugar this adds justifies itself. I.e. I'm not convinced yet that it's improvement over wrapping the call to
setState
inisMounted
. They are both one extra concept in the code that you always need to remember to do.This
defer
is nicer in that the handler doesn't have to concern itself with with the awkward state of running while unmounted. But doing it that way also increases the "reading distance" between thesetState
call and where the guard protects it. This muddies things for the reader, IMO.Are there use cases for this other than silencing the error React throws when when setting state on an unmounted component? That might convince me further.
This also makes me think React shouldn't error in this situation. It should probably just warn, though maybe I'm not understanding the value of having it error. Either way though, the discussion and code above boils down to essentially turning errors on or off for that situation. Except we're doing it in a way that's mixed up with other component logic.
Maybe a better fix would be something like
React.createClass({...}, {errorOnUmountedSetState: false})
to turn it off for the class. Or perhaps a way to turn it off globally.Then the app-specific and interesting logic in the component isn't cluttered with keeping what are honestly lint-level warnings from crashing the program.
6434712
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
facebook/react#1247 (comment)
6434712
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll step in - have to agree with @dmnd, this sugar is just additional layer of abstraction that should be present for every asynchronous call. Same as adding
isMounted
check in all handlers.Only benefit that I'm seeing: logical consistency - when component is destroyed, it is guaranteed that there are no background operations waiting for that component. Unless someone forgot to register the deferred.
6434712
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrjoes that's exactly right - the hope was that you would use this for any deferred operations, rather than noticing at some point that you're calling operations on an unmounted component. The problem is I think we're trading one thing to remember for another.