- Notifications
You must be signed in to change notification settings - Fork1.1k
fix(site): fix react state violation in filetree create/update utils#20483
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Closes#20482I've just replaced `set` from `lodash` with `set` from `lodash/fp`. Wecould alternatively just ensure `fileTree` is cloned before using themutable version.
BrunoQuaresma left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That is interesting. Could we create a test for that? To make sure we don't have any regressions in the future?
Parkreiner left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The change itself is right – I'm just worried about what the style of change means for the larger codebase
site/src/utils/filetree.ts Outdated
| } | ||
| returnset(fileTree,path.split("/"),value); | ||
| returnset(path.split("/"),value)(fileTree); |
ParkreinerOct 27, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I get the benefits of opting for the FP version, especially since it handles immutability out of the box, but I'm worried about bringing new conventions into the codebase without having any conversations about them first
The rest of the codebase leans into a lot more vanilla JS-based patterns, and while currying like this works just fine, I'm worried about consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd be leaning towards making a copy of the value using traditional JS methods, and then using the vanilla Lodashset method on that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah I'm more than happy to do that 👍
- Replace use of `lodash/fp/set` with `lodash/cloneDeep` followed by`lodash/set`.- Fix `removeFile` as that still did mutation.- Update tests to assert that the original FileTree was not modified.
DanielleMaywood commentedOct 27, 2025
@BrunoQuaresma I've added a test to verify the @Parkreiner I've replaced the usage of |
Uh oh!
There was an error while loading.Please reload this page.
40fc337 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Closes#20482
Ensure file tree utils don't modify their parameters so we don't violate Reacts rules