Skip to content

add(method)[FE-896]: Add method for executing code without listening for popstate#13

Merged
zachpwr merged 1 commit into
masterfrom
fe/zachary.power/add-popstate-pause-method
Apr 4, 2019
Merged

add(method)[FE-896]: Add method for executing code without listening for popstate#13
zachpwr merged 1 commit into
masterfrom
fe/zachary.power/add-popstate-pause-method

Conversation

@zachpwr
Copy link
Copy Markdown

@zachpwr zachpwr commented Apr 4, 2019

Summary:

Background:

nuclear-router listens for popstate events. The browser emits these events when the "Back" button is pressed or when the application executes window.history.back(). nuclear-router handles these events by re-evaluating the URL and executing the corresponding route handler.

The Optimizely application uses URL hashes (/my-page#dialog) to represent when a full page dialog is being shown. This allows the user to close the full page dialog by hitting the "Back" button on their browser. However, in order to close a full page dialog in any other scenario (i.e. the user clicks "Submit" or "Cancel"), the application calls window.history.back() to close the dialog and remove the hash from the URL (/my-page#dialog -> /my-page).

When the application performs this navigation, nuclear-router re-evaluates the route handler for that page even though removing the URL hash does not change the underlying route being accessed. This can potentially cause unnecessary API calls and initialization work to be executed.

Implementation:

This PR adds a method to the Router interface called executeWithoutPopstateListener. It accepts one parameter, fn. This method allows the consuming application to run code without the popstate listener active. The code to be executed without the popstate listener enabled is placed in the argument fn.

This will allow the Optimizely application to close dialogs using window.history.back() without the router re-evaluating the route for the page underneath the dialog. For example:

function closeDialog() {
    router.executeWithoutPopstateListener(() => {
        /**
         * This will not cause the router to re-evaluate since
         * it's wrapped in this function
         */
        window.history.back();
    });
}

Test Plan:

This PR adds a test case to ensure that the popstate listener is deactivated while executing the function passed to executeWithoutPopstateListener.

@zachpwr zachpwr self-assigned this Apr 4, 2019
@zachpwr zachpwr requested a review from a team as a code owner April 4, 2019 16:24
@zachpwr zachpwr force-pushed the fe/zachary.power/add-popstate-pause-method branch from 17e0f31 to 1ee1a77 Compare April 4, 2019 18:54
@zachpwr zachpwr changed the title DO NOT REVIEW! [FE-896] Add method for executing code without listening for popstate add(method)[FE-896]: Add method for executing code without listening for popstate Apr 4, 2019
@zachpwr zachpwr force-pushed the fe/zachary.power/add-popstate-pause-method branch from 1ee1a77 to ff35c92 Compare April 4, 2019 21:03
Comment thread test/Router.spec.js Outdated
@jamesopti
Copy link
Copy Markdown

This approach requires the caller to know the side effect of window.history.back (in your example) as a wrapper around that method.

What if instead we configured nuclear-router to ignore hash changes?

That could be a bit overkill, but are there any reasons we'd want to re-route on pop-state of a hash change?

@zachpwr zachpwr force-pushed the fe/zachary.power/add-popstate-pause-method branch from f0aa8aa to 4cfb0df Compare April 4, 2019 21:19
@zachpwr zachpwr requested a review from jordangarcia April 4, 2019 21:20
@jordangarcia
Copy link
Copy Markdown

@jamesopti that could be a reasonable way to go about it, but it seems a lot more risky. What in our app invisibly relies on route re-evaluation for hash changes, I definitely don't know where these happen off the top of my head.

My vote is for a more gradual approach, where we have to explicitly change the behavior by using this new function. This approach seems like it de-risks such a global change.

Copy link
Copy Markdown

@jordangarcia jordangarcia left a comment

Choose a reason for hiding this comment

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

🐑 💨

@zachpwr zachpwr merged commit e580d3b into master Apr 4, 2019
Comment thread src/Router.js
*/
executeWithoutPopstateListener(fn) {
this.__shouldHandlePopstateEvents = false;
fn();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Might there be a reason to return this result to the caller?

@jamesopti
Copy link
Copy Markdown

@jamesopti that could be a reasonable way to go about it, but it seems a lot more risky. What in our app invisibly relies on route re-evaluation for hash changes, I definitely don't know where these happen off the top of my head.

My vote is for a more gradual approach, where we have to explicitly change the behavior by using this new function. This approach seems like it de-risks such a global change.

Fair. But I'd imagine @zachpwr plans to introduce this in the dialog manager's hideDialog method, which I'd guess is 99% of our hash usage.

@zachpwr zachpwr deleted the fe/zachary.power/add-popstate-pause-method branch April 5, 2019 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants