Prevent duplicate event handler registration by configuring the router only once

[jsnajdr]

Remove the options parameter from page.create/createPage (it wasn't used anyway) and also from the Page constructor together with the configure call there.

That ensures that the router is configured only once, with the user-supplied options, instead of being configured twice: first with default options in constructor, second time with user-supplied options in page.start.

Fixes a real-world issue where passing { click: false } to page.start didn't prevent adding the global click handler to the document anyway, because the default value of the click option is true.

How I tested this:
Unit tests still pass both in Node and in browser.

I also tested the router in WordPress.com Calypso and it works as expected. The issues described in

• [WIP] Prevent duplicated event bindings for page instances #509 (comment)
• Update dependency page to v1.11.1 Automattic/wp-calypso#28071 (comment)
• Boot: Don't render empty Layout Automattic/wp-calypso#26944

are no longer present and everything works as expected again 👌

Based on earlier PR by @kaisermann (#509)
Fixes #508

[coveralls]

Coverage decreased (-0.03%) to 92.083% when pulling c4f7264 on jsnajdr:fix/duplicate-handler-registration into 57a9c31 on visionmedia:master.

[kaisermann]

@matthewp When and if you merge this, could you update the version on the package.json as well? It's not up to date.

[matthewp]

Yup!

[andyburke]

This seems to be causing an issue for me around page._window not being defined now that .configure() is not being called. Does that make any sense? I have not dug deeply on this yet.

[andyburke]

Just verified that things are working for me as expected in 1.11.0, but are breaking due to _window being undefined in 1.11.1.

[andyburke]

Seems to be required to call page.start() in 1.11.1. Does there need to be a major version bump here?

[jsnajdr]

Hi @andyburke, do you use the library without ever calling page.start at all? As I understand it, calling page.start was always mandatory. The library wouldn't work without this initialization call before version 1.10 (the one that introduced page.create, then it accidentally worked from 1.10 until now -- but that caused problems. This PR fixed these problems and page.start is mandatory again.

Does that help? I'd like to learn more about how exactly you use page.js, because the this._window reference error shouldn't happen in any valid usage scenario that I'm aware of. Do you have a call stack telling us in which function this error happens?

[andyburke]

I would call things like:

• page.base()
• page( '*', ... )
• page.exit( '*', ... )

before eventually calling page( url ) to start routing. With 1.11.1 I was suddenly hitting page._window not being defined, whereas I had not had that before.

Unfortunately, I already switched things around to use page.start() in my code so I could make progress, so I don't have a call stack for you.

[jsnajdr]

@andyburke Defining route handlers before page.start is OK, but calling page( url ) (which is an alias for page.show( url ) without page.start is not. I just verified that it indeed crashes as you describe in 1.11.1, and also that is crashes exactly the same way in 1.9.0. That was the version before the page.create refactoring.

[andyburke]

Actually, now I am struggling with this... I'm doing a page.start( { dispatch: false } ), but then calling page.show( url ) and it is not dispatching, I think due to this change:

2447b23#diff-50e3e6872ec225cd4caa566f1f15e89cR603

Why is the 'dispatch' argument to page.show() ignored in preference to page instance's _dispatch?(which I think should indicate initial dispatch, not all dispatching, right?)

[andyburke]

btw ... why am I always the one with dispatching issues?: d5c92c1

[jsnajdr]

Why is the 'dispatch' argument to page.show() ignored in preference to page instance's _dispatch?

I agree that's a bug. page.show should look at the dispatch argument, like page.replace does.

this._dispatch should be used only in page.start to determine whether to perform the initial dispatch, i.e., to look at the current window.location and call the appropriate route hander.

It doesn't even need to be an instance property visible to everyone. Local variable inside page.start would be sufficient and less error-prone.