jaredpalmer / formik

Build forms in React, without the tears 😭
https://formik.org
Apache License 2.0
34.02k stars 2.79k forks source link

Fix bug with setNestedObjectValues #3693

Closed cjauvin closed 1 year ago

cjauvin commented 2 years ago

I'm aware that this project is seemingly no longer maintained, but I'd like to propose this fix for https://github.com/jaredpalmer/formik/issues/3674, in case it's useful to someone.

The problem with the current version of the setNestedObjectValues function is that if you're using objects for your form values (instead of strings) and they happen to be identical (in my case I'm using an object like {name: "English", iso639: "en"} with two distinct Material UI Autocomplete widgets), the fact that visited is implemented as a WeakMap will cause trouble, because the values will collide. The result is that your touched object (which is what is being computed by this function) will miss certain values for certain fields.

The first commit in this PR is actually my first solution to this problem, which was to replace the WeakMap by a Set, and use the "path" into the object structure as the keys. So if you have a value structure like { names: ["aa", "bb"]}, the paths names.0 and names.1 would be used as keys.

But then I realized that the visited mechanism is not needed in this context, as there is no need to prevent any infinite recursion, as the value object structure is necessarily a tree, and not a graph. I might be wrong about this last analysis, as I could not test extensively, but I simply cannot see any reason for this mechanism whatsoever. If I'm wrong for any reason, the solution is very easy: revert my second commit, and simply keep the first.

johnrom commented 2 years ago

The problem that visited solves is when objects have a circular reference. Think about person1 having a list of friends and person2 having a list of friends. If they both consider each other friends, it looks like this function will iterate through these objects over and over again in an infinite loop. However, it checks to see if an object was visited before doing so.

However, I recommend overall circumventing the issue by not containing references to objects in form values, and instead creating a plain value with an identifier that can map to the original object. If there are plain shallow objects that are being used twice, like a default value, I would { ...spread } them, both for separation of concerns and because it is unlikely this patch would land any time soon based on the state of this project.

cjauvin commented 2 years ago

Thanks you made me realize that my understanding of WeakMap was actually flawed, because I somehow thought that distinct but identical objects would map to the same key. That is not the case I see it now, and my original problem is that I'm indeed using the same plain shallow object for more than one form values. I'm going to spread them as you suggest.

About your last remark, I'm curious: so this popular project is really abandoned, not maintained anymore? Would it be somehow possible to transfer its maintenance to some other party you think? I'm curious about what happens when a project with this size and popularity suddenly goes dark, wouldn't it be nice if there was a mechanism to federate its community toward a solution? By "solution" here, I mean something like: we commonly agree that this fork X and these people Y will be the new "official" maintainers, and it will be clearly stated somewhere, to avoid confusion for newcomers and outsiders.

johnrom commented 2 years ago

I had a personal fork of it, but I'm probably going to try react hook form instead just because I don't have the bandwidth to maintain a project of this size. In terms of what should happen, I have no idea. If I try react hook form and it isn't to my liking, I may consider circling back, but I have a feeling it will work as a suitable replacement.

In terms of what I would recommend, I would say that users of open source projects (or any third party dependencies) should prepare for the event when any of their dependencies loses maintenance, and should build their projects with modularity in mind -- keeping all the dependent code which connects to Formik and other libraries abstracted from the majority of their codebase as much as possible. I think it's possible that:

But ultimately, who knows what will happen, and I have learned to keep libraries like this abstracted from my components when possible.