scottrippey / next-router-mock

Mock implementation of the Next.js Router
MIT License
401 stars 38 forks source link

mocked next/link behavior seems inconsistenst with next/link from next.js when the `as` prop is used #89

Closed sebaholesz closed 1 year ago

sebaholesz commented 1 year ago

In our codebase we use the logic of dynamic paths and query parameters to create a more complex abstraction on top of the basic next.js routing logic. In essence, we have a couple of specific pages between we navigate using a next/link like in the example below:

<NextLink
    as={isStatic ? as : href} // isStatic resolves to false, so as=href
    prefetch={false}
    href={
        isStatic
            ? href
            : {
                  pathname: FriendlyPagesDestinations[type],
                  query: { [SLUG_TYPE_QUERY_PARAMETER_NAME]: FriendlyPagesTypes[type], ...queryParams },
              }
    }
    {...props}
>
    {children}
</NextLink>

If I navigate using the following link:

<ExtendedNextLink type="category" href="/test-category">
    link text
</ExtendedNextLink>,

the actual implementation of next/link shows me the following properties:

{
    asPath: "/test-category".
    pathname: "/categories/[categorySlug]", // this is caused by the dynamic pathname shown above
    query: { 
        slugType: "front_product_list" // also caused by the dynamic query above) 
    },
}

but the mocked one gets overwritten because of the as prop and by this logic in the mock (specifically in MemoryRouter):

const newRoute = parseUrlToCompleteUrl(url, this.pathname);
let asPath;
let asRoute;
if (as === undefined || as === null) {
    asRoute = undefined;
    asPath = getRouteAsPath(newRoute.pathname, newRoute.query, newRoute.hash);
}
else {
    asRoute = parseUrlToCompleteUrl(as, this.pathname);
    asPath = getRouteAsPath(asRoute.pathname, asRoute.query, asRoute.hash);
}

My question is, am I misunderstanding something? According to my understanding, if I navigate using a next/link where I paste arbitrary URL and query, the as prop should not overwrite it, as it should only modify the asPath property of the router. Is this a bug, or a feature?

scottrippey commented 1 year ago

I think I need a little more detail to understand the problem. Could you explain what you DO get from mockRouter when testing that link?

Next does do something kinda surprising, so maybe this explains the problem.
Take a look at these tests, they describe the behavior seen in Next:

https://github.com/scottrippey/next-router-mock/blob/main/src/MemoryRouter.test.tsx#L369-L381

~Basically, the href is used as the "visible" route (in the address bar), whereas the as parameter is used as the "real" route (think filesystem). So the router values will all reflect the as route details, and you don't really know what the "visible" route is.~

~I think, based on your example, you need to switch the as and href. I hope this helps!~

sebaholesz commented 1 year ago

Hey Scott, thanks for getting back to me.

To answer your questions, if I call it on mockRouter, I get this:

{
    asPath: "/test-category".
    pathname: "/test-category"
    query: { },
}

When it comes to the as prop, I think we are not agreeing on how it works in Next.js. If I understand you correctly, you say that href is visible and as is used in the background. However, according to my understanding, it works exactly the other way around. According to this snippet from the Next.js docs: image the as prop serves as the visible mask in the browser bar. I also base my opinion on the behavior I was able to see.

To simulate my behavior, you will only need 3 files in the /pages directory with the following contents:

index.tsx

import NextLink from 'next/link';

const Index = () => {
    return (
        <>
            <h1>index</h1>
            <NextLink href="/test-category?foo=bar" as="/my-pretty-url">
                test category
            </NextLink>
        </>
    );
};

export default Index;

my-pretty-url.tsx

import { useRouter } from "next/router";

const MPU = () => {
    const router = useRouter()
    return (
        <>
            <h1>my pretty url</h1>
            <p>{JSON.stringify(router)}</p>
        </>
    );
};

export default MPU;

and lastly, test-category.tsx

import { useRouter } from "next/router";

const TC = () => {
    const router = useRouter()
    return (
        <>
            <h1>test category</h1>
            <p>{JSON.stringify(router)}</p>
        </>
    );
};

export default TC;

If you then click on the link in index.tsx, the test-category.tsx component will be rendered, but the URL will say /my-pretty-url. Furthermore, this is what the router will look like:

{
    pathname: "/test-category",
    route:"/test-category",
    query: {
        foo: "bar"
    },
    asPath: "/my-pretty-url",
    resolvedAs: "/my-pretty-url"
}

I don't know if we are on the same page. Let's figure that one first before talking about if the mockRouter behaves correctly.

scottrippey commented 1 year ago

Thank you for researching this. What you described does sound correct, and aligns with the Next docs. I must have misunderstood when I read the docs initially. I haven't used as in a production app.

I'd be happy to update this mock's behavior to match. You could help by creating a PR with tests, if you're up for it!

sebaholesz commented 1 year ago

Thanks for the response. I would be open to creating a PR. Do you have a guideline for PRs? What should be included, etc.?

scottrippey commented 1 year ago

I don't have any strict guidelines around PRs. The project uses Prettier and has CI checks for everything important, so hopefully it's easy to start contributing. If you want to start by adding/updating unit tests to capture the correct, intended behavior, that's a great start!

scottrippey commented 1 year ago

I created a CodeSandbox to test the Next behavior, and verified that it's just as you described. https://codesandbox.io/p/sandbox/priceless-davinci-68js6q

scottrippey commented 1 year ago

@sebaholesz I've got a PR ready for this! #94 Can you please review the PR, ESPECIALLY the unit tests, and verify that those tests capture the correct behavior as you'd expect to see in the app? I created an example-app so that I could verify the correct Next behavior, and I'm fairly confident that I've captured the correct logic in the unit tests.

sebaholesz commented 1 year ago

Hey Scott! Sorry for disappearing for a couple of days. I will do the CR today.

sebaholesz commented 1 year ago

Not sure if I can fully review the hash behavior and compare it to the actual next router. This is probably next-router-mock specific. However, the rest of it seems good to me. I have tested the behavior locally with our pretty complex router setup and it all matches as far as I can see. Great job and thanks for taking the time!