Providing "search" key in URL objects for Next links drops parameter interpolation

tatethurston commented 2 years ago

Verify canary release

[X] I verified that the issue exists in Next.js canary release

Provide environment information

Operating System:
  Platform: darwin
  Arch: x64
  Version: Darwin Kernel Version 20.6.0: Wed Jan 12 22:22:42 PST 2022; root:xnu-7195.141.19~2/RELEASE_X86_64
Binaries:
  Node: 16.14.0
  npm: 8.3.1
  Yarn: 1.22.18
  pnpm: N/A
Relevant packages:
  next: 12.1.1
  react: 17.0.2
  react-dom: 17.0.2

What browser are you using? (if relevant)

N/A

How are you deploying your application? (if relevant)

N/A

Describe the Bug

Providing a search key in URL objects passed to Nextjs links drops parameter interpolation.

Eg the following:

      <Link
        href={{
          pathname: "/days/[day]",
          query: { day: "foo" },
          search: "foo=bar"
        }}
      >
        Day
      </Link>

Will render a link to /days/[day]?foo=bar.

Expected Behavior

From the next/link documentation:

You can use every property as defined in the Node.js URL module documentation.

      <Link
        href={{
          pathname: "/days/[day]",
          query: { day: "foo" },
          search: "foo=bar"
        }}
      >
        Day
      </Link>

Should render a link to /days/foo?foo=bar.

To Reproduce

Provide a search key in a URL objects passed to a Link.

https://codesandbox.io/s/relaxed-germain-eupjpe

tatethurston commented 2 years ago

Thanks for taking a look @balazsorban44 -- this API is especially convenient when you want to protect against any potential key collision between query terms and search terms, eg:

      <Link
        href={{
          pathname: "/days/[day]",
          query: { day: "foo" },
          search: "day=some-other-value"
        }}
      >
        Day
      </Link>

This may seem contrived, but becomes increasingly likely as various tracking search terms are passed across application boundaries. If the above were written:

      const search = getSearch(...) // evaluates non obviously to `day: 'some-other-value'`
      <Link
        href={{
          pathname: "/days/[day]",
          query: { day: "foo", ...search },
        }}
      >
        Day
      </Link>

We would clobber the page query term and endup on a page like /days/some-other-value

SukkaW commented 2 years ago

We would clobber the page query term and end up on a page like /days/some-other-value

If you prefer day: 'foo' to be overwritten:
```
query: { day: "foo", ...search }
```
If you prefer day: 'foo' not to be overwritten:
```
query: { ...search, day: "foo" }
```

So IMHO it is not a problem here.

tatethurston commented 2 years ago

We would clobber the page query term and end up on a page like /days/some-other-value

If you prefer day: 'foo' to be overwritten:
query: { day: "foo", ...search }
If you prefer day: 'foo' not to be overwritten:
query: { ...search, day: "foo" }
So IMHO it is not a problem here.

@SukkaW the ask here is to avoid anything from being overwritten, not to choose which value is overwritten. If there is a name collision between the path segment name and search parameter key (3rd party analytics, or something else) then it's not acceptable for either key to be overwritten.

vercel / next.js