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

Don't decode paths in locations (keep the browser-native value) #69

Closed
jacobwgillespie opened this issue Mar 21, 2019 · 3 comments
Closed

Comments

@jacobwgillespie
Copy link

Hey, I'm really excited about Navi and the potential to replace React Router with it in one of our apps. This is a feature request, and I'm not sure how difficult it would be given Navi's architecture and its use of the history module, but here we go:

It would be awesome if Navi could bypass history's automatic decoding of location.pathname, discussed in this issue here: remix-run/history#505.

That automatic decoding deviates from browser behavior and makes the flow of pathnames very difficult to reason about - it results in divergent behavior when navigating directly to a page via a URL vs when navigating through history with back/forward vs when clicking on a <Link>. We have worked around this by creating a custom <PageRoute> component that manually decodes path params from window.location, but it's all very hack-y.

It would be cool to just drop the entire mess and switch to Navi, however if I'm following the code correctly, I think that bug would still apply here since it's internal to history. If there's a way for Navi to either provide a workaround that doesn't depend on history for the path params, or for Navi to drop the dependency on history entirely (like Reach Router does), that would be really awesome!

@jamesknelson
Copy link
Collaborator

This is definitely something we can do.

In the next release, I'm hoping to remove the dependency on history (while still providing the option to pass in a history object for compatibility with react-router).

If we remove the history, we'll still need some code to handle the same tasks, but the new code can definitely address the issue you've mentioned.

If you'd like to help out, a PR that removes the History dependency by handling it all within Navi would be super helpful.

@jacobwgillespie
Copy link
Author

Looks like history just merged remix-run/history#656, which removes the automatic decoding, about two hours ago. It's not yet released, but just FYI the next major version of history will have this behavior omitted.

@jamesknelson
Copy link
Collaborator

Okay. Will keep this open until the next major version of history is released, and bump Navi's version then too.

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

No branches or pull requests

2 participants