The Wayback Machine - https://web.archive.org/web/20201018181514/https://github.com/vercel/next.js/issues/17840
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

[Next 9.5.4+] Router.push duplicates query params in production mode, but not in dev mode #17840

Open
CarsonMcKinstry opened this issue Oct 13, 2020 · 4 comments

Comments

@CarsonMcKinstry
Copy link

@CarsonMcKinstry CarsonMcKinstry commented Oct 13, 2020

Bug report

Describe the bug

I recently came across this bug while using the "new" router.push method. Using the following

router.push({
    pathname: '/posts/[pid]',
    query: {
        pid: '1234',
        foo: 'bar'
    }
})

In dev, this directs me to /posts/1234?foo=bar

In prod, this directs me to /posts/1234?pid=1234&foo=bar

To Reproduce

Steps to reproduce the behavior, please provide code snippets or a repository:

  1. clone and install this repo
  2. run yarn build
  3. run yarn start
  4. navigate to localhost:3000
  5. press The Button

Expected behavior

I would have expected production to run the same as in dev mode. Like I said above, pushing with this information

{
    pathname: '/posts/[pid]',
    query: {
        pid: '1234',
        foo: 'bar'
    }
}

Should send me to /posts/1234?foo=bar.

System information

  • OS: macOS
  • Browser (if applies): chrome
  • Version of Next.js: 9.5.4, 9.5.5
  • Version of Node.js: 12.16.1
@ShoeBoom
Copy link

@ShoeBoom ShoeBoom commented Oct 14, 2020

I can work on this.

@ShoeBoom
Copy link

@ShoeBoom ShoeBoom commented Oct 17, 2020

It seems that this bug exists as far back as version 9.0.0. At this point, I think the best solution is the document this behavior and fix the development version to comply instead.

Thoughts?

@erikdstock
Copy link
Contributor

@erikdstock erikdstock commented Oct 17, 2020

I spent about two hours faffing with this, particularly the resolveHref method but ultimately couldn't get a consistent environment for debugging, logging, writing a test or making sense of the type coercion (like the selected lines above where we return a string or an array of strings). Probably started too deep trying to write a unit test for this function in retrospect. I was trying to find out why we only call these lines if resolveAs is passed:

    if (
          isDynamicRoute(finalUrl.pathname) &&
          finalUrl.searchParams &&
          resolveAs  // I think this is usually undefined when called from router.push()?
        ) {
      // throw new Error('HERE ' + finalUrl.search)
      const query = searchParamsToUrlQuery(finalUrl.searchParams)
      const { result, params } = interpolateAs(
        finalUrl.pathname,
        finalUrl.pathname,
        query
      )

      if (result) {
        interpolatedAs = formatWithValidation({
          pathname: result,
          hash: finalUrl.hash,
          query: omitParmsFromQuery(query, params),  // the only time we call omitParamsFromQuery()
        })
      }
    }
// ... execution continues until:
    return (resolveAs  // again, appears to be undefined
      ? [resolvedHref, interpolatedAs || resolvedHref] // confused about returning an array here
      : resolvedHref) as string

Maybe a red herring but this was my suspicion - hard to debug with so many assumptions built in.

@ShoeBoom
Copy link

@ShoeBoom ShoeBoom commented Oct 18, 2020

@erikdstock @toolmantim I think I know the source of the bug. Here

Now I'm wondering whether we should fix the prod or the dev version due to backwards compatibility

It seems to occurs due to resolvedAs having different values in prod and dev

    let resolvedAs = as   <-- "/posts/[pid]?pid=1234&foo=bar"

    if (process.env.__NEXT_HAS_REWRITES) {
      // this code is only run in dev
      // here dev will set resolvedAs  to "/posts/[pid]"
      resolvedAs = resolveRewrites(
        parseRelativeUrl(as).pathname, <------------------ "/posts/[pid]"
        pages,
        basePath,
        rewrites,
        query,
        (p: string) => this._resolveHref({ pathname: p }, pages).pathname!
      )

      if (resolvedAs !== as) {
        const potentialHref = removePathTrailingSlash(
          this._resolveHref(
            Object.assign({}, parsed, { pathname: resolvedAs }),
            pages,
            false
          ).pathname!
        )

        // if this directly matches a page we need to update the href to
        // allow the correct page chunk to be loaded
        if (pages.includes(potentialHref)) {
          route = potentialHref
          pathname = potentialHref
          parsed.pathname = pathname
          url = formatWithValidation(parsed)
        }
      }
    }
    // in dev resolvedAs = "/posts/[pid]" & and in prod resolvedAs = "/posts/[pid]?pid=1234&foo=bar"
    resolvedAs = delLocale(delBasePath(resolvedAs), this.locale)

This is because inside resolveRewrites

export default function resolveRewrites(
  asPath: string,
  pages: string[],
  basePath: string,
  rewrites: Rewrite[],
  query: ParsedUrlQuery,
  resolveHref: (path: string) => string
) {
  if (!pages.includes(asPath)) { <------------------- this is false
    // ....
  }
  return asPath <---------- // returns /posts/[pid]
}

changing this to let resolvedAs = parseRelativeUrl(as).pathname fixes the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.