search

visionmedia / page.js

Micro client-side router inspired by the Express router
http://visionmedia.github.com/page.js
7.67k stars 687 forks source link

Prevent duplicate event handler registration by configuring the router only once #519

Closed jsnajdr closed 5 years ago

jsnajdr commented 5 years ago

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

are no longer present and everything works as expected again 👌

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

coveralls commented 5 years ago

Coverage Status

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

kaisermann commented 5 years ago

@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 commented 5 years ago

Yup!

andyburke commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

jsnajdr commented 5 years ago

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 commented 5 years ago

I would call things like:

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 commented 5 years ago

@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 commented 5 years ago

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:

https://github.com/visionmedia/page.js/commit/2447b2313887c95ab2aeea56433192677b174a3a#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 commented 5 years ago

btw ... why am I always the one with dispatching issues?: https://github.com/visionmedia/page.js/commit/d5c92c1c6c5652875ebee7d4bb55fa402d47be48

jsnajdr commented 5 years ago

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.