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

Make query string parameters work when using hashbang routing #551

Open 5im-0n opened 4 years ago

5im-0n commented 4 years ago

When hashbang is true, urls like http://localhost/?foo=bar got redirected to http://localhost/?foo=bar#!?foo=bar. This duplicated the query string and made the url ugly. This commit fixed this problem by making urls like http://localhost/?foo=bar redirect to http://localhost/#!?foo=bar

I made some nightwatch tests to make sure all use cases are covered. The tests can be found here: https://github.com/S2-/page.js-nightwatch-test

All urls work as expected:

hashbang: false

navigating to http://127.0.0.1:8001/ -> http://127.0.0.1:8001/
clicking a link href="/hello" -> http://127.0.0.1:8001/hello
clicking a link href="/hello?some=query&str=ing" -> http://127.0.0.1:8001/hello?some=query&str=ing
navigating to http://127.0.0.1:8001/?some=query&str=ing -> http://127.0.0.1:8001/?some=query&str=ing
navigating to http://127.0.0.1:8001/hello?some=query&str=ing -> http://127.0.0.1:8001/hello?some=query&str=ing

hashbang: true

navigating to http://127.0.0.1:8000/ -> http://127.0.0.1:8000/
clicking a link href="/hello" -> http://127.0.0.1:8000/#!/hello'
clicking a link href="/hello?some=query&str=ing" -> http://127.0.0.1:8000/#!/hello?some=query&str=ing'
navigating to http://127.0.0.1:8000/?some=query&str=ing -> http://127.0.0.1:8000/#!?some=query&str=ing'
navigating to http://127.0.0.1:8000/?some=query&str=ing#!/hello -> http://127.0.0.1:8000/#!/hello?some=query&str=ing'
coveralls commented 4 years ago

Coverage Status

Coverage increased (+0.06%) to 92.623% when pulling 3c1ca83ba1efc3adce9279ccd398a9d2ac4b4a36 on S2-:qs-on-hash into 94138d1f3105f9fdb3424f9c0870c1fb90adcd1a on visionmedia:master.

matthewp commented 4 years ago

Thanks! Any chance you can add a test?

5im-0n commented 4 years ago

No sorry, I don't have mocha skills and no interest in learning yet an other testing framework. I hope the existing passing mocha tests are enough.

matthewp commented 4 years ago

Unfortunately there's been issues in the past where a fix with no test broke another case that also was untested. So we've made it a policy to only merge pull requests with tests, to prevent regressions.

5im-0n commented 4 years ago

Seems a good policy. I just don't have any mocha skills 🤷‍♂️