[FE-896] Make executeWithoutPopstateListener asynchronous#14
Conversation
| this.__shouldHandlePopstateEvents = true; | ||
|
|
||
| // Execute the function, then re-enable the popstate listener | ||
| return fn().then((result) => { |
There was a problem hiding this comment.
Why not just use .finally? It requires the consumer to pass an ES6 Promise (not a Deferred) but I think thats OK/preferred.
return fn().finally(function() {
this.__shouldHandlePopstateEvents = true;
});There was a problem hiding this comment.
Good idea! I can add that in instead.
There was a problem hiding this comment.
Actually, had to switch back to the original setup (.then(resolveHander, rejectHandler)). .finally was giving CI some trouble since it requires polyfill.
There was a problem hiding this comment.
@jamesopti I thought of that too (using .finally) but we might impact other companies that are using nuclear-router without an ES7-spec Promise polyfill that ensures .finally is available.
| this.__shouldHandlePopstateEvents = true; | ||
|
|
||
| // Execute the function, then re-enable the popstate listener | ||
| return fn().then((result) => { |
There was a problem hiding this comment.
I imagine you are using the two function arguments with .then for best cross compatibility with jQuery Deferreds and Promises? This is probably best since this is a module used by potentially disparate types of FE clients.
There was a problem hiding this comment.
True that it would make this more compatible with deferreds, but considering the following reasoning, we can probably go with .finally as James has suggested:
- In the
.then(resolveHander, rejectHandler)setup, I was still returningPromise.<resolve|reject>(), so it would still add some weirdness if using deferreds with this - Promises are preferred as we are trying to move away from jQuery deferreds
There was a problem hiding this comment.
As we discussed offline, I'm reverting back to the original solution (.then(resolveHander, rejectHandler)) since the lack of .finally polyfill causes CI tests to fail.
zachpwr
force-pushed
the
fe/zachary.power/make-popstate-blocker-async
branch
from
b6599bf to
8f57ac1
Compare
zachpwr
force-pushed
the
fe/zachary.power/make-popstate-blocker-async
branch
from
8f57ac1 to
5125fc4
Compare
3 participants
Summary:
The method
executeWithoutPopstateListenerwas introduced in #13 to allow for executing code without thepopstateevent listener firing innuclear-router.After re-examining the use case for this method, it was discovered that events which emit
popstateevents, such as hitting the "Back" button or callingwindow.history.back(), almost always queue the page navigation to occur asynchronously. Therefore, this PR changesexecuteWithoutPopstateListenerto accept an asynchronous function. This PR also now returns thePromisechain that is handled in this method.Test Plan:
This PR modifies the unit test for this method to use
Promises rather than synchronous execution.