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

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

Closed
sebaholesz opened this issue Jul 26, 2023 · 9 comments · Fixed by #94 or #102

Comments

@sebaholesz
Copy link

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?

@sebaholesz sebaholesz changed the title mocked next/link behavior seems inconsistenst with next/link from next.js mocked next/link behavior seems inconsistenst with next/link from next.js when the as prop is used Jul 26, 2023
@scottrippey
Copy link
Owner

scottrippey commented Jul 27, 2023

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
Copy link
Author

sebaholesz commented Jul 31, 2023

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

scottrippey commented Aug 9, 2023

@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
Copy link
Author

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

@sebaholesz
Copy link
Author

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!

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

Successfully merging a pull request may close this issue.

2 participants