[WIP] Prevent duplicated event bindings for page instances

[kaisermann]

Fixes #508

I've removed the this.configure() call from the Page constructor (and the options argument) since it's already called and used at page.start.

(Maybe just passing the options throught the .create() would be enough, but it wouldn't solve the duped events in case of the .start being also called at some point)

[coveralls]

Coverage decreased (-2.1%) to 91.597% when pulling c4d9db5b13f45d6394ff7039a5adb83c6f5acd1a on kaisermann:fix-duplicated-events into 02c026ca82f9d3f5018038d822592e77c040cfb7 on visionmedia:master.

[matthewp]

Thanks for contributing! Just so I understand, the problem is that calling configure() multiple times results in multiple _onpopstate event bindings? I'm surprised to hear that calling addEventListener multiple times with the same event name and function results in that function being called multiple times.

Please confirm that I am understanding correctly. If so I think we should solve in a different way. We want configure() to be able to be called multiple times. Someone might (for some non-obvious reason) call configure() before calling start(). It should set the configuration to the latest configure() call but obviously only register one event listener. So perhaps some boolean flag (ugh) is necessary to know if the event has already been registered.

[kaisermann]

@matthewp Funny, I'm making some tests regarding multiple addEventListener calls with the same event and callback and it seems only one is registered 🤔. I will try to find out what's happening and come back here.

Thanks for your attention and quick response (this lib is very important for my job hehe 😁)!

[kaisermann]

Okay, the problem is that the globalPage is also binding to the click/popstate because the this.configure is called in the constructor. I've tried to call .stop() before creating my Page instance, but the this._running isn't set yet, so .stop() does nothing.

Maybe moving the binding part of .configure() to the .start() (since we already have this._click and this._popstate to use anywhere) or something like to a ._bindEvents() would be okay?

Current workaround is to call .start() to set _running = true and then call .stop():

  import Page from 'page';

  Page.start();
  Page.stop();
  const _pageInstance = Page.create();
  _pageInstance.start();

[matthewp]

Could you describe how you are using page.create()? Usually it is intended to control another window, like an iframe, and keep the global page for the normal window. It sounds like maybe you are using your pageInstance to control the same page that the global page already does, is that correct? If you could describe the use case I think we can come up with a good solution. I hadn't considered multiple page instances controlling the same window before.

[kaisermann]

I have an application which can load a webapp (in same window object) that uses Page. However, when this webapp is unloaded/destroyed, .stop() ._callbacks=[], etc is not enough to reset the global Page. I'm basically using it to be able to "reset"/"restart" the router.

[matthewp]

Ok, thanks for that info. Based on that I think the problem might be in .stop() then. I'll try to get some time later in the week to look into this.

[kaisermann]

Thanks! Just let me know if I can help with anything 😁

[matthewp]

If you have time, what is the reason that calling stop() on the global page doesn't prevent it from handling navigation? Is there some event we are not unbinding from?

[kaisermann]

Because it checks for this._running before unbinding, which is set ONLY in .start().

That's why I suggested to move the binding part to the .start() method. Wouldn't it make sense, since the .stop() unbind the events, to the .start() to the the opposite?

[jsnajdr]

We stumbled on this issue when trying to update github.com/Automattic/wp-calypso to the latest 1.11.0 version. There are a few conceptual issues with the page.create() API that allows to create multiple router instances for multiple windows.

Which function configures the router with options? Both page.create and page.start take the options parameter and call configure. The page.create function, however, ignores the options and configures the router with default options. After calling page.start, with the desired options, the router gets configured for the second time. Happens both for the default singleton router and for user-created instances, too.

Solution: make clear distinction between what page.create and page.start are expected to do.

• page.create( window ) should only have a window parameter, create the router, bind the methods, and set the window that the router manages.
• page.start( options ) does the rest of the initialization, everything that depends on options should be done there.
• page.stop() should be able to completely shut down the router to the state before page.start. It should be identical to the state after page.create, ready to be page.start-ed again.

@kaisermann @matthewp What do you think? I can help with rebasing and updating this PR to implement what I described above.

[matthewp]

@jsnajdr I think that sounds right. Assuming that can be done in a non-breaking way I'm cool with that solution!

[jsnajdr]

@matthewp @kaisermann I created a PR with the fix as proposed: #519

page.create() has no parameters and doesn't configure the router. page.start( options ) is the only method that receives the options and configures the router accordingly.

[matthewp]

Cool, I merged that. Releasing.

[kaisermann]

I've just realized why Page.callbacks = [] is not enough to "reset"/"destroy" the global Page (or any other) instance.

The instance callbacks array is passed to the Page object here. When someone tries to empty it with Page.callbacks = [], it simply rewrites the reference of the instance.callbacks to a new empty array. If anyone needs to empty the callbacks array, I suggest a Page.callbacks.length = 0.