Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes internal routing of URLs with query strings #217

Merged
merged 1 commit into from
May 25, 2017

Conversation

lukejacksonn
Copy link
Contributor

@lukejacksonn lukejacksonn commented May 25, 2017

app(
  view: {
    '/:a/:b': _ => h('button', { onclick: a.navigate }, 'Click Me')
  },
  actions: {
     navigate: (s,a,d) => a.router.go('/other/path?query=1')
  },
  plugins: [Router],
)

Given the above app being served from localhost (with history-api-fallback behavior). If you were to visit the URL http://localhost:8080/some/path?query=0 everything would work as expected:

  • The router would match /:a/:b from location.pathname on startup and render the view.

You would be able to see a button. Clicking on the button fires a.router.go which gets passed a URL in the same form as the one matched on startup.. this time:

  • No view is rendered because no match was found.

This is because when the page loads router.match gets passed the pathname part of the current URL (which evaluates to /some/path) whereas router.go tries to match the string it is passed without parsing out the pathname part (/other/path?query=1 instead of /other/path).

This PR is trying to ensure only the pathname part of a URL passed to router.go gets passed to router.match so that you can internal route to URLs with query parameters.

@codecov
Copy link

codecov bot commented May 25, 2017

Codecov Report

Merging #217 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #217   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         164    164           
  Branches       43     43           
=====================================
  Hits          164    164
Impacted Files Coverage Δ
src/router.js 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c12c292...2c32676. Read the comment docs.

@jorgebucaran
Copy link
Owner

@lukejacksonn Can't users split("?") themselves?

@lukejacksonn
Copy link
Contributor Author

Then you lose that data from the url because it would not go through:

history.pushState({}, "", data)

which would mean if someone were then to refresh the newly visited page, the data that was in query string would no longer be accessible.

@jorgebucaran jorgebucaran merged commit 5b5b7c7 into jorgebucaran:master May 25, 2017
@jorgebucaran
Copy link
Owner

Thanks @lukejacksonn! 🎉

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.

None yet

2 participants