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: warning each child should have a unique key #1820

Merged
merged 2 commits into from
Dec 9, 2024

Conversation

ahmadfsalameh
Copy link
Contributor

Checklist

  • only relevant code is changed (make a diff before you submit the PR)
  • run tests npm run test
  • tests are included
  • commit message and code follows the Developer's Certification of Origin

@adrai @aiktb @XAOPT When I made the changes in #1813 I wasn't aware of the conditional return, It seems in the example provided here: https://stackblitz.com/edit/vitejs-vite-59g4bx?file=package.json the first if statement in TransWithoutContext is returning void causing the unmodified components (without key) to get rendered.

That function handles both arrays and objects using Object.keys which is smart but also very confusing. We could revert my changes and the warning will disappear but it will cause errors to show up because those components are readonly, also as specified here in your type: https://github.com/i18next/react-i18next/blob/master/TransWithoutContext.d.ts#L16.

In this pull request I'm attempting to solve both issues, mutating the component prop and the keys warning. I split the function into smaller functions which will return a new components value at the end without overriding it.

It might also fix #1733

@coveralls
Copy link

coveralls commented Dec 7, 2024

Coverage Status

coverage: 96.782% (-0.3%) from 97.08%
when pulling 815ff0f on ahmadfsalameh:master
into 37cf812 on i18next:master.

src/TransWithoutContext.js Outdated Show resolved Hide resolved
@aiktb
Copy link
Contributor

aiktb commented Dec 8, 2024

This looks ok, I tested it locally with the patched code and it successfully fixed the warning.

@aiktb
Copy link
Contributor

aiktb commented Dec 8, 2024

Can you add a test for this? @adrai This does not need to be done in this PR
I tried upgrading the react-related development dependencies to 19, which did not break any tests, but some peer dependencies were broken. Upgrading the following dependencies eliminated the peer dependency errors and all tests passed:

npm install @testing-library/react@^16.1.0 @types/react@^19.0.0 react@^19.0.0 react-dom@^19.0.0 react-test-renderer@^19.0.0
- "@testing-library/react": "^16.0.0",
+ "@testing-library/react": "^16.1.0",

- "@types/react": "^18.3",
+ "@types/react": "^19.0.0",

- "react": "^18.3.1",
+ "react": "^19.0.0",

- "react-dom": "^18.3.1",
+ "react-dom": "^19.0.0",

- "react-test-renderer": "^18.3.1",
+ "react-test-renderer": "^19.0.0",

@adrai
Copy link
Member

adrai commented Dec 8, 2024

Can you add a test for this? @adrai This does not need to be done in this PR

I tried upgrading the react-related development dependencies to 19, which did not break any tests, but some peer dependencies were broken. Upgrading the following dependencies eliminated the peer dependency errors and all tests passed:

npm install @testing-library/react@^16.1.0 @types/react@^19.0.0 react@^19.0.0 react-dom@^19.0.0 react-test-renderer@^19.0.0
- "@testing-library/react": "^16.0.0",

+ "@testing-library/react": "^16.1.0",



- "@types/react": "^18.3",

+ "@types/react": "^19.0.0",



- "react": "^18.3.1",

+ "react": "^19.0.0",



- "react-dom": "^18.3.1",

+ "react-dom": "^19.0.0",



- "react-test-renderer": "^18.3.1",

+ "react-test-renderer": "^19.0.0",

Can you create a PR for the deps update?

@aiktb
Copy link
Contributor

aiktb commented Dec 8, 2024

Can you add a test for this? @adrai This does not need to be done in this PR
I tried upgrading the react-related development dependencies to 19, which did not break any tests, but some peer dependencies were broken. Upgrading the following dependencies eliminated the peer dependency errors and all tests passed:

npm install @testing-library/react@^16.1.0 @types/react@^19.0.0 react@^19.0.0 react-dom@^19.0.0 react-test-renderer@^19.0.0
- "@testing-library/react": "^16.0.0",

+ "@testing-library/react": "^16.1.0",



- "@types/react": "^18.3",

+ "@types/react": "^19.0.0",



- "react": "^18.3.1",

+ "react": "^19.0.0",



- "react-dom": "^18.3.1",

+ "react-dom": "^19.0.0",



- "react-test-renderer": "^18.3.1",

+ "react-test-renderer": "^19.0.0",

Can you create a PR for the deps update?

#1821

@adrai
Copy link
Member

adrai commented Dec 8, 2024

Can you add a test for this?

@ahmadfsalameh 👆

@adrai adrai merged commit 85349f9 into i18next:master Dec 9, 2024
8 of 9 checks passed
@adrai
Copy link
Member

adrai commented Dec 9, 2024

merged and released with v15.1.4... please @ahmadfsalameh if possible create a new PR with a dedicated test for this

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 this pull request may close these issues.

Trans component should NOT modify the input props value.
4 participants