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

fix(youtube-embed): each child in a list should have a unique "key" prop #57579

Merged
merged 5 commits into from
Oct 31, 2023

Conversation

RodrigoTomeES
Copy link
Contributor

Hi,

I added a key in the map iteration of scripts in Youtube Embed. I use the index as a key but maybe there are a better solution about this like script.url + index 🤔

There isn't open related issues with this PR as this moment.

      {scripts?.map((script, index) => (
        <Script
          key={index}
          src={script.url}
          strategy={scriptStrategy[script.strategy] as ScriptProps['strategy']}
          stylesheets={stylesheets}
        />
      ))}

@MartinGK
Copy link

We don’t recommend using indexes for keys if the order of items may change. This can negatively impact performance and may cause issues with component state.

src

@RodrigoTomeES
Copy link
Contributor Author

RodrigoTomeES commented Oct 27, 2023

We don’t recommend using indexes for keys if the order of items may change. This can negatively impact performance and may cause issues with component state.

src

But in this situation, will the order of the items be changed? In that case, what key do you suggest?

Also, I saw in the documentation that if you don't pass a key React use the index as a key but this throw an error when you use the library. So maybe pass the index as a key doesn't have much sense and there is another way to remove the error 🤔

@dimaMachina
Copy link

in that case, what key do you suggest?

just script.url?

@RodrigoTomeES
Copy link
Contributor Author

in that case, what key do you suggest?

just script.url?

But could it be repeated, right? Wouldn't script.url + index be better?

@dimaMachina
Copy link

But could it be repeated, right? Wouldn't script.url + index be better?

probably in real life, it should never be repeated, I don't see the case when you should show the same video twice or more

@RodrigoTomeES
Copy link
Contributor Author

But could it be repeated, right? Wouldn't script.url + index be better?

probably in real life, it should never be repeated, I don't see the case when you should show the same video twice or more

Maybe you are right. In addition, it could warn you that you have a repeated video that you have not realized about.

@huozhi huozhi added the CI approved Approve running CI for fork label Oct 30, 2023
@ijjk
Copy link
Member

ijjk commented Oct 30, 2023

Stats from current PR

Default Build
General
vercel/next.js canary RodrigoTomeES/next.js patch-2 Change
buildDuration 10.5s 10.4s N/A
buildDurationCached 6s 5.9s N/A
nodeModulesSize 175 MB 175 MB
nextStartRea..uration (ms) 414ms 419ms N/A
Client Bundles (main, webpack)
vercel/next.js canary RodrigoTomeES/next.js patch-2 Change
199-HASH.js gzip 30 kB 30 kB N/A
3f784ff6-HASH.js gzip 53.2 kB 53.2 kB
494.HASH.js gzip 182 B 182 B
framework-HASH.js gzip 45.5 kB 45.5 kB
main-app-HASH.js gzip 254 B 252 B N/A
main-HASH.js gzip 33.1 kB 33.1 kB N/A
webpack-HASH.js gzip 1.75 kB 1.75 kB N/A
Overall change 98.9 kB 98.9 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary RodrigoTomeES/next.js patch-2 Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary RodrigoTomeES/next.js patch-2 Change
_app-HASH.js gzip 205 B 205 B
_error-HASH.js gzip 182 B 181 B N/A
amp-HASH.js gzip 505 B 507 B N/A
css-HASH.js gzip 322 B 323 B N/A
dynamic-HASH.js gzip 2.59 kB 2.59 kB N/A
edge-ssr-HASH.js gzip 258 B 259 B N/A
head-HASH.js gzip 348 B 347 B N/A
hooks-HASH.js gzip 369 B 368 B N/A
image-HASH.js gzip 4.38 kB 4.38 kB N/A
index-HASH.js gzip 256 B 256 B
link-HASH.js gzip 2.67 kB 2.67 kB N/A
routerDirect..HASH.js gzip 318 B 318 B
script-HASH.js gzip 384 B 383 B N/A
withRouter-HASH.js gzip 319 B 320 B N/A
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 885 B 885 B
Client Build Manifests
vercel/next.js canary RodrigoTomeES/next.js patch-2 Change
_buildManifest.js gzip 484 B 484 B
Overall change 484 B 484 B
Rendered Page Sizes
vercel/next.js canary RodrigoTomeES/next.js patch-2 Change
index.html gzip 527 B 529 B N/A
link.html gzip 540 B 543 B N/A
withRouter.html gzip 523 B 523 B
Overall change 523 B 523 B
Edge SSR bundle Size
vercel/next.js canary RodrigoTomeES/next.js patch-2 Change
edge-ssr.js gzip 96.1 kB 96.1 kB N/A
page.js gzip 140 kB 140 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary RodrigoTomeES/next.js patch-2 Change
middleware-b..fest.js gzip 624 B 626 B N/A
middleware-r..fest.js gzip 148 B 151 B N/A
middleware.js gzip 23 kB 23 kB N/A
edge-runtime..pack.js gzip 1.92 kB 1.92 kB
Overall change 1.92 kB 1.92 kB
Commit: 136d301

@kodiakhq kodiakhq bot merged commit df67fa1 into vercel:canary Oct 31, 2023
51 of 56 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI approved Approve running CI for fork locked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants