Manage circular references in objects #104
Conversation
|
Thanks for working on this. This needs a test. |
|
Ok I'll work on that, thanks. I also just saw that there is an error with the linting, but I don't really understand why. |
Fixed in |
|
I've added a test to reproduce the error and to validate the new code. (Your changes on main are all visible in my pull-request now, I don't really understand why, but I read it seems to be a normal behavior in github.) |
| proxy.layers[1].group.layers[0].group.layers[1].group.layers[1].group.layers[2].val = 33; | ||
| t.is(resultPath, 'layers.2.val'); | ||
| }); | ||
|
|
There was a problem hiding this comment.
This test does not test the mentioned infinite loop. It would be good with a test that would have caused infinite loop before this pull request.
There was a problem hiding this comment.
Sorry, actually "infinite loop" was not the right term here.
The more we were modifying the object property, the more the path was getting longer.
This mean that after the first call of callback we got proxy.layers[1].group.layers[0].val.
After the second call we got proxy.layers[1].group.layers[0].group.layers[2].group.layers[0].val.
After the third call we got proxy.layers[2].group.layers[2].group.layers[1].group.layers[0].group.layers[2].group.layers[0].val.
... and so on.
Example fo logs:
And after 1 minute of usage of our app:
And actually we reached a memory limit because we were getting path that has Megabytes of data.
So, we cannot say this is a infinite loop.
But this tests actually exactly covers our problem.
There was a problem hiding this comment.
Ok. I would reword the comment then.
There was a problem hiding this comment.
Ok, I've rewrited the comment.
index.js
Outdated
| // and the path can get longer and longer until we reach a memory limit. | ||
| const childPath = path.concat(basePath, property); | ||
| const existingPath = cache.getPath(value); | ||
| if (existingPath && !isArray(childPath) && !isSymbol(childPath) && childPath.toString().startsWith(existingPath.toString())) { |
There was a problem hiding this comment.
Using startsWith is a bit fragile. It could potentially match the wrong thing. For example, existingPath is foo while childPath is foo2.bar.
There was a problem hiding this comment.
Yes, you're totaly right, I didn't thought about this. I've rewriten the code to do a better comparison.
Actually I didn't manage the arrays of paths correctly in my first pull-request. I changed the code to manage arrays as well. And I've added another unittest for that too.
That's a sign of incorrect rebasing. Your commit repeats: https://github.com/sindresorhus/on-change/pull/104/commits |
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
Should I do something here ? |
|
Thanks for merging, and have a merry Christmas ! 😊 |
|
Merry Christmas :) |
Hello,
I analyzed the problem described in the issue #103 more in details, and I think this Pull-Request shoulb solve the problem.
The solution is to look first if the object already has a path within the same object, and if yes, not to generate a new path.
So actually if we are looking for the parth
group.children[0].children[0].parentthe object behind this path actually already exists with the pathgroup.children[0], and it's exactly the same object within the same tree structure. Therefore, the shortest path to the object should be used.It's still possible to have the same object under many different path, this change in the code is only managing circular references within the same object.
The issue and the new code contain more comments and explanations.
Fixes #103
Could you please have a look at it and tell me if this change is ok for you?
Thanks !
And I forget the most important : Many thanks for your job, this library is really useful for us :)