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

Duplicate <link rel="canonical"/> #32944

Closed
ekanant opened this issue Jan 2, 2022 · 6 comments
Closed

Duplicate <link rel="canonical"/> #32944

ekanant opened this issue Jan 2, 2022 · 6 comments
Labels
bug Issue was opened via the bug report template.

Comments

@ekanant
Copy link

ekanant commented Jan 2, 2022

What version of Next.js are you using?

12.0.4

What version of Node.js are you using?

v16.10.0

What browser are you using?

Chrome

What operating system are you using?

macOS

How are you deploying your application?

next start

Describe the Bug

Hi, Core Team

I develop a website with next.js and I have to do about SEO, so I study from a guideline https://nextjs.org/docs/api-reference/next/head.

I put a default meta tag in custom _app.tsx with <title/> or meta tag and some page that have to specific <title/> or meta I use key property to avoid the duplication it work fine, but when I put a link tag for canonical. Output show a html content with duplicate canonical.

//_app.tsx
import Head from 'next/head'
import type { AppProps } from "next/app";
const MyApp = ({ Component, pageProps }: AppProps) => {
       const { asPath } = useRouter();
       return (
             <>
                   <Head>
                        <link rel="canonical" key="canonical" href={asPath}/>
                  </Head>
                  <Component {...pageProps} />
             </>
       )
}
//pages/example.tsx
const Example: React.FC<{}> = () => {
     return (
          <>
                <Head>
                        <link rel="canonical" key="canonical" href="yyyyyyy"/>
                  </Head>
          </>
     )
}

Example html output

<head>
     <link rel="canonical" href="/default"/>   <!-- This from _app.tsx-->
     
     <link rel="canonical" href="/example"/>   <!-- This from pages/example.tsx-->
</head>

So I go to the shared/lib/head.js https://github.com/vercel/next.js/blob/canary/packages/next/shared/lib/head.tsx#L67, I found a unique logic by

if (h.key && typeof h.key !== 'number' && h.key.indexOf('$') > 0) {
          
            hasKey = true;
            const key = h.key.slice(h.key.indexOf('$') + 1);
            if (keys.has(key)) {
                isUnique = false;
            } else {
                keys.add(key);
            }
        }

If I set the key with $ in the key for an example.

//_app.tsx
 <link rel="canonical" key="my_key$canonical" href="yyyyyyy"/>
//pages/example.tsx
 <link rel="canonical" key="my_key$canonical" href="yyyyyyy"/>

The canonical from pages/example.tsx is output as one and only canonical for that page.

I'm not sure why you have to check h.key.indexOf('$') > 0, I think most of developer follow the guide from https://nextjs.org/docs/api-reference/next/head, If the logic was right, May be the document need to be updated or this is a new bug.

Please answer ASAP because I want to fix this as fast as possible, I have to keep working for an seo on website.

Thanks in advance.

Expected Behavior

Expected the with key props not duplicate for an example .

To Reproduce

Create a with key property and put it in Head from next/head The will duplicate in head tag.

//_app.tsx
import Head from 'next/head'
import type { AppProps } from "next/app";
const MyApp = ({ Component, pageProps }: AppProps) => {
       const { asPath } = useRouter();
       return (
             <>
                   <Head>
                        <link rel="canonical" key="canonical" href={asPath}/>
                  </Head>
                  <Component {...pageProps} />
             </>
       )
}
//pages/example.tsx
const Example: React.FC<{}> = () => {
     return (
          <>
                <Head>
                        <link rel="canonical" key="canonical" href="yyyyyyy"/>
                  </Head>
          </>
     )
}

Example html output

<head>
     <link rel="canonical" href="/default"/>   <!-- This from _app.tsx-->
     
     <link rel="canonical" href="/example"/>   <!-- This from pages/example.tsx-->
</head>
@ekanant ekanant added the bug Issue was opened via the bug report template. label Jan 2, 2022
@balazsorban44
Copy link
Member

Given your reproduction, I could not reproduce this. Make sure you don't have a typo in key.

@ekanant
Copy link
Author

ekanant commented Jan 3, 2022

Hi, @balazsorban44

I just found the will fuplicate If I use with preact (https://github.com/preactjs/next-plugin-preact)

I create a repo with duplicate result
https://github.com/ekanant/nextjs-duplicate-in-head

I set

key="canonical"

The is duplicate.

but If I set

key="my_key$canonical"

The is not duplicate

@balazsorban44
Copy link
Member

It seems to be an issue with next-plugin-preact then. Please take it up in https://github.com/preactjs/next-plugin-preact

@ekanant
Copy link
Author

ekanant commented Jan 3, 2022

@balazsorban44
OK, but I'm doubt about the logic in unique function why is has to check h.key.indexOf('$') > 0 because it make key="test-key" always false (-1 > 0), key="test-key$canonical" is return true, I think if (h.key && typeof h.key !== 'number' ) {} may be enough.

if (h.key && typeof h.key !== 'number' && h.key.indexOf('$') > 0) {
          
            hasKey = true;
            const key = h.key.slice(h.key.indexOf('$') + 1);
            if (keys.has(key)) {
                isUnique = false;
            } else {
                keys.add(key);
            }
        }

@adhirajsinghchauhan
Copy link

I'm doubt about the logic in unique function why is has to check h.key.indexOf('$') > 0

That's from #9315 and it's necessary because React "escapes" keys by prepending .$ while remapping children into an array.
Relevant lines of code (facebook/react@a724a3b5/.../ReactChildren.js):

const SEPARATOR = '.'; // line 22
...

function escape(key: string) {
  ...
  return '$' + escapedString; // line 41
}

function getElementKey(element: any, index: number) {
    ...
    return escape('' + element.key); // line 71
    ...
}

function mapIntoArray() {
    ...
    const childKey =
      nameSoFar === '' ? SEPARATOR + getElementKey(child, 0) : nameSoFar; // line 116
    ...
}

In Preact, children is already an array; no remapping necessary. So I'm assuming it's just dumped onto DOM without changing values. Quote from https://preactjs.com/guide/v10/differences-to-react#children-api:

For Preact this is generally unnecessary, and we recommend using the built-in array methods instead. In Preact, props.children is either a Virtual DOM node, an empty value like null, or an Array of Virtual DOM nodes. The first two cases are the simplest and most common, since it's possible to use or return `children as-is

I see you've created preactjs/next-plugin-preact#51, but I'm not sure if that's the right repository. Perhaps preact/compat is where such adjustments need to be made, since that's the Preact->React compat layer. Anyway, for now I've manually put .$ before all my keys (similar to what you did with my_key$), and NextJS' dedupe logic works.

@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Feb 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue was opened via the bug report template.
Projects
None yet
Development

No branches or pull requests

3 participants