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: Attempted to assign to readonly property #1813

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

ahmadfsalameh
Copy link
Contributor

@ahmadfsalameh ahmadfsalameh commented Nov 22, 2024

TypeError: Attempted to assign to readonly property.

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

Checklist (for documentation change)

  • only relevant documentation part is changed (make a diff before you submit the PR)
  • motivation/reason is provided
  • commit message and code follows the Developer's Certification of Origin

@adrai
Copy link
Member

adrai commented Nov 22, 2024

Can you provide the original issue for this PR?
Or at least describe with more detail information what this PR exactly should fix?

@coveralls
Copy link

Coverage Status

coverage: 97.082%. remained the same
when pulling b02ffef on ahmadfsalameh:master
into c9a73eb on i18next:master.

@adrai
Copy link
Member

adrai commented Nov 25, 2024

Can you provide the original issue for this PR? Or at least describe with more detail information what this PR exactly should fix?

@ahmadfsalameh can you provide more information please?

@ahmadfsalameh
Copy link
Contributor Author

@adrai A few of our users got this error. When I investigated the stack trace, it led me to https://github.com/i18next/react-i18next/blob/master/src/TransWithoutContext.js#L365, where a recent pull request (#1806) introduced code that mutates the components passed in the props to add missing keys. This is an incorrect approach according to the React docs.

You must treat React elements and their props as immutable and never change their contents after creation. In development, React will freeze the returned element and its props property shallowly to enforce this.

Screenshot 2024-11-25 at 3 26 20 PM

@adrai
Copy link
Member

adrai commented Nov 25, 2024

Can you create a reproducible example that triggers this error?
Or even better, add a test case?

@adrai adrai merged commit 2d00f34 into i18next:master Nov 26, 2024
8 of 9 checks passed
@aiktb
Copy link
Contributor

aiktb commented Dec 7, 2024

The changes in this PR are wrong. comp is just a temporary variable abbreviation. It makes no sense to assign it to cloneElement and add a key, so it will definitely lead to #1805 reopen.

I cannot reproduce the problem mentioned in this PR using react@19 and [email protected].

@adrai I don't understand how to solve #1805 if you can't modify the components in props. No one wants to add a key to every i18n component. It's not even an array.

@adrai
Copy link
Member

adrai commented Dec 7, 2024

@ahmadfsalameh can you help please?
If not, we're going to revert your pr.

@aiktb
Copy link
Contributor

aiktb commented Dec 7, 2024

@adrai I also noticed that react does freeze props in development mode, but it's a shallow freeze, you can even run the following code, so what does this PR mention have to do with the changes in #1805? idk

const obj = {
    props: {
        comp: {}
    }
};
Object.freeze(obj.props);
obj.props.comp.key = "value";

console.log(obj.props.comp.key === "value");

@adrai
Copy link
Member

adrai commented Dec 7, 2024

@aiktb can you provide a new PR that reverts this...? if @ahmadfsalameh can create a new PR if he founds a better way to address his issue.

@aiktb
Copy link
Contributor

aiktb commented Dec 7, 2024

@aiktb can you provide a new PR that reverts this...? if @ahmadfsalameh can create a new PR if he founds a better way to address his issue.

My network environment is very bad now, and I can do it when I wake up in 8 hours

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.

4 participants