Ensure comparison of routes is tolerant of an initially undefined query

[baer]

In the situation where query parameters are used but are not a part of the original URL, Next.js will throw an error. This happens when this.query is compared against the new query object using the shallow-compare function which expects Objects as its input.

See below:

    // Current URL is www.my-company.com/user

    const router = this.props.router;
    const href = `${router.pathname}?searchQuery=${value}`;
    router.replace(href, href, { shallow: true });

I considered altering shallow compare but decided it was better to put this in the router for two reasons.

1. Since in JS, everything is an object, ensuring the type was valid in shallow-compare.js would be a lot of code for a little benefit. That is unless you wanted to pull in lodash.

2. shallowCompare like anything can only be so defensive. The onus is on the caller to pass in the right params. Since the function is really shallowObjectCompare and we're passing undefined, I felt like the correct thing to do was to pass in a valid object.

[timneutkens]

Could you add a test for this? Thanks 🙏

[baer]

🤔 💭 The above code only breaks when there is a custom server defined. Looks like it's related to #2943. I'd like to see if I can figure out what the root cause is here since this will only move the problem in #2943 to a new spot - maybe one that is harder to see.

[baer]

Okay, I added a test case but running the suite crashed my computer with all of the webdriver procs. I know that CI will verify that the test is good but I'd like to also check locally. Is there a cmd for running the unit tests only?

[timneutkens]

I wonder in what case this happens 🤔 Could you also add an integration test

[baer]

This happens in the case described in #2943.

I'm not sure I see the value in a full on integration test. They tend to be hard to run, somewhat flaky and hard to maintain so I've always favored saving them to smoke test critical paths. This was my experience from working on the Walmart team and with the https://github.com/TestArmada/magellan project anyway. I'm happy to write another unit test if you can think of other ways to cause this behavior.

If you feel strongly that an integration test is the right move here, can you give me some guidance on running the test suite? It nearly crashed my computer and left a bunch of orphaned Chrome processes around last time. Is there a way to do more targeted runs?

[baer]

Any updates on how you'd like me to handle this?

[timneutkens]

@baer to confirm, the only use case you're trying to fix is:

If app.render(req, res, pathname, query) is called and query is not an object, there will be a nasty error that will happen when calling Router.push (or any other router method that internally calls change).

So that app.render(req, res, pathname) works?

[baer]

The bug I'm up against manifests in a slightly different way but yes, that's the one. After looking a bit more it looks like there are two problems, and this PR only solves one of them:

Problem 1

The following custom server which throws the URL error every time I try to shallow route and will not update the URL. If I render to static, the problem goes away and the URL will update just fine.

The client-side code causing the error

const router = this.props.router;
const href = `${router.pathname}?searchQuery=${value}`;
router.replace(href, href, { shallow: true });

The custom server

const next = require("next");
const express = require("express");

const port = parseInt(process.env.PORT, 10) || 4000;
const dev = process.env.NODE_ENV !== "production";
const app = next({ dev });
const handle = app.getRequestHandler();

app
  .prepare()
  .then(() => {
    const server = express();

    server.get("/customer/:id", (req, res) => {
      const actualPage = "/customer";
      const queryParams = { id: req.params.id };
      app.render(req, res, actualPage, queryParams);
    });

    server.get("*", (req, res) => {
      return handle(req, res);
    });

    server.listen(port, err => {
      if (err) throw err;
      // eslint-disable-next-line no-console
      console.log(`> Ready on http://localhost:${port}`);
    });
  })
  .catch(err => {
    // eslint-disable-next-line no-console
    console.error(err.stack);
    process.exit(1);
  });

Problem 2

I can make the error go away with the following code in the custom server (where I handle the URL parsing myself) but on static render, the router.query object will always be empty!

server.get("/customers", (req, res) =>
  app.render(req, res, "/customers", parse(req.url, true).query)
);

In a static rendered app with the following URL I'll get this

http://localhost:4000/customers?searchQuery=bla

So, if I'm trying to use something like componentDidMount or getDerivedStateFromProps to restore state from the URL, I get undefined even though the asPath does contain the query.

---

It appears as if the url isn't parsed at some point in the chain. On the server side, I can I can fix the problem by parsing it myself for every route which leads me to think that it's the handleRequest function that appears to be behaving badly by not forwarding on query parameters.

[baer]

Update: In the client side, the Router constructor is getting an undefined for query but valid values for as and pathname AND there being a query param attached to pathname. It's like it doesn't get parsed.

I tried to trace through the code but I'm not sure where to follow this thread. Any help would be appreciated.

[timneutkens]

@baer so, since query can be custom when pre-rendering (SSR/static export) it is sent with the initial payload, in case of static exporting this means that the query gets hardcoded on export.

[baer]

So if I navigate to a statically rendered site, the query will be set to undefined if it was not included in the export? Is the below example a correct understanding:

Export
/customers

URL
https://my-cdn.com/customers?filter=bla

Despite the query in the path, the Router in the client should receive undefined as the query?

[timneutkens]

Yeah that's currently the case. Although it should be {} instead of undefined, which sounds like there's a bug somewhere in static exporting 🤔 / the dev server exportPathMap read.

I'm open to changing this behavior as it might come unexpectedly. Though right now it ensures server/client consistency.

[baer]

When you say consistency I am assuming you mean that when React re-hydrates, the client and server DOM needs to match or it throws warnings all over the place. This is tricky...

I know it's not ideal but maybe checking for queryParams and pushing a new route onto the stack would work well. I know it'd cause a re-render (and in some cases a big one) but it seems like a small price to make the client and server side behaviors match.

Also, thanks for all of the guidance here! This has been really productive. In the interim, I've coded around this behavior by parsing the asPath manually.

  componentDidMount() {
    if (process.browser) {
      // Set the initial serach state if one is provided in a query parameter
      this.setState({
        searchValue: getQueryParams(this.props.router.asPath).searchQuery
      });
      ...

[timneutkens]

I've created a PR that solves at least one of the cases you outlined in this thread 👍

When you say consistency I am assuming you mean that when React re-hydrates, the client and server DOM needs to match or it throws warnings all over the place. This is tricky...

Yeah, that's exactly what I meant. Right now we favor consistency when re-hydrating to the correctness of the URL properties. Like I said before I can definitely see use cases where it'd be nice to have the query defined at runtime when statically exporting.

Also, thanks for all of the guidance here! This has been really productive.

Thank you very much for saying this, much appreciated 😌

Thanks for being patient, this is the main reason we don't flat out merge every PR immediately and ask for the case you're trying to solve. The solution in this PR would hide the more structural bugs like the one in #4466 👍

[baer]

Thanks for that PR and, more generally, for everything y'all do!

I believe this gets me past the URL error but there is still the inconsistency between the Router's internal state and the URL on static renders. You mentioned preferring DOM consistency to URL consistency but what are the trade-offs in triggering a second paint (if the URL is different)?

If y'all hate the 2nd paint idea, maybe adding an API to the router to force a re-eval in userland. Something roughly equivalent to:

Router.replace(this.props.router.asPath, this.props.router.asPath, { shallow: true })