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

Navigation's behavior on search is not clear #808

Open
govizlora opened this issue Jun 25, 2020 · 5 comments
Open

Navigation's behavior on search is not clear #808

govizlora opened this issue Jun 25, 2020 · 5 comments

Comments

@govizlora
Copy link

I found the behavior of navigation is different from v4 to v5:

For example, I'm currently at /foo/?id=1. When I call history.push({ pathname: '/bar' }),

  • In v5, the url will be /bar/?id=1
  • In v4, the url will be /bar

I'm not sure if this behavior is added explicitly, or by accident, since in https://github.com/ReactTraining/history/blob/master/docs/navigation.md it is not documented.

Personally, I prefer the way v5 handles it, and I'm planning to utilize this feature in my own app. But I'm afraid that the behavior is unintentional and will be removed in the near future, so I'm hoping that it can be documented.

@brookback
Copy link

I'm also experiencing this, but I don't think it's that intuitive behaviour :D

Pushing a new path to the stack shouldn't keep the old query parameter, imo, since it'll feel like a "hangover state", and I as the app developer must remember to manually clear the query part when I'm doing navigations.

If this is kept as it is now, a query parameter can potentially be hanging around throughout the app's navigation if it's never cleared. Smells like a bug to me.

@brookback
Copy link

FYI, it's in the getNextLocation function:

function getNextLocation(to: To, state: State = null): Location {
        return readOnly<Location>({
            ...location,
            ...(typeof to === 'string' ? parsePath(to) : to),
            state,
            key: createKey(),
        });
    }

If to is a simple string here, it won't overwrite the search that is spread from location, since parsePath will only return a { pathName: string } value (no search).

In parsePath function, one could set search to be present by empty instead of not present at all:

if (searchIndex >= 0) {
  partialPath.search = path.substr(searchIndex);
  path = path.substr(0, searchIndex);
+} else {
+  partialPath.search = '';
+}

@mlegenhausen
Copy link

mlegenhausen commented Jul 1, 2020

I'm also experiencing this, but I don't think it's that intuitive behaviour :D

Would confirm that this is counter intuitive. When I don't set a search in an url string I don't want one.

My current workaround that should be backwards compatible when a change in this logic should occur is

const [pathname, search] = url.split('?')
history.push({ pathname, search })

mlegenhausen pushed a commit to werk85/effe-ts that referenced this issue Jul 1, 2020
@davidsavoie1
Copy link

I'm also experiencing this, but I don't think it's that intuitive behaviour :D

Would confirm that this is counter intuitive. When I don't set a search in an url string I don't want one.

My current workaround that should be backwards compatible when a change in this logic should occur is

const [pathname, search] = url.split('?')
history.push({ pathname, search })

Not a bad idea, however, it's pretty tricky since it would break if, for some reason, many '?' are present in the url. It doesn't take "#" into account either.

To me, this is definitively a bug or at least a breaking change... I'll have to stick to a prior version for a library that depends on it until I understand how to work with this behaviour.

@unbugx
Copy link

unbugx commented Jan 15, 2021

I'm also experiencing this, but I don't think it's that intuitive behaviour :D

Would confirm that this is counter intuitive. When I don't set a search in an url string I don't want one.

My current workaround that should be backwards compatible when a change in this logic should occur is

const [pathname, search] = url.split('?')
history.push({ pathname, search })

According to documentation search should has initial '?' otherwise it will not work
https://github.com/ReactTraining/history/blob/master/docs/api-reference.md#locationsearch

my final solution:

const [pathname, search] = url.split('?')
history.push({ pathname, search: search ? `?${search}` : '' })

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

5 participants