fix: dset doesnt deepmerge
#1804
Conversation
🦋 Changeset detectedLatest commit: 4f2bba3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report
@@ Coverage Diff @@
## main #1804 +/- ##
=======================================
Coverage 65.66% 65.66%
=======================================
Files 85 85
Lines 5115 5115
Branches 1624 1624
=======================================
Hits 3359 3359
Misses 1752 1752
Partials 4 4
Continue to review full report at Codecov.
|
|
@maraisr thanks for this! looks like theres a bug with the implementation according to the cypress suite, where only two payload items made it through, did you notice that? something seems to be broken |
| keys: string | ArrayLike<string | number>, | ||
| value: V, | ||
| ): void; | ||
| export function dset(obj, keys, val) { |
There was a problem hiding this comment.
@lukeed with your blessing I have in-lined this for now, once dset/merge if/when becomes a think I'll update this.
|
I believe it is very important that this new utility comes with unit tests to cover all the use cases, especially the deep merge use case. |
|
Agreed. At least the bare minimum for utility tests. When I polish this up as a standalone (soon) I'll have extensive coverage. In summary, arrays and objects are merged. On any positional or key/index conflict, the newer value is taken. For non-object values, the new value is always given preference. |
|
For transparency sake, @maraisr reached out to me about this and I provided the dset-merger implementation to be inlined since I don't have the bandwidth to make it it's own module atm. If there are any issues with it please let me know |
Ya this is true. But the idea here isnt that external people consume this utility. Its analogous to using an npm package. The use-cases are covered in the e2e. This is a safe bet as it stands now, which once Luke's package drops we should be certain. Till this is is still 100x better than what's currently in mainline. But happy for this PR to stagnate till this has dropped—because I still believe we need even more e2e's to cover all of the edge cases. But for this, I need to change the way its tested, and introduce a "unit test" for this multipart stuff, so that yeah we can fire fine-grained events and assert the state. Waiting for your call on this one @acao |
|
ohh i see. this is what you meant by inlining |
|
I was looking for an alternative, and found
|
|
Deep-set, dset, set-value, lodash.set are all the effectively the same thing. They set/assign a value at a nested path. The difference here is that you guys dont want an assignment. You want to merge/collect values at a nested path. I know of no module that does this, hence the inlined fork of Recommend |
|
Published |
|
@acao @imolorhe I have updated this branch with the new This also means that graphiql will now correctly articulate the results of an Incrementally Delivered response.
|
|
Nice! Approved. Looks like changeset is missing? |
|
@imolorhe I think there is a changesets already in this pr. Feel free to assist me on the wording: https://github.com/graphql/graphiql/pull/1804/files#diff-10046eb3e404bb78fb2100699c84af52ebd1593ada28eea0318bd0a548c0cb60 |
|
Oh okay I missed that. My bad. |
Upon testing this feature further, I got aware that
dsetwouldn't "deep merge" the payloads. Which is only prevalent with the@deferdirective. As up until now@streamwouldn't only append to an array—whereas@deferwould merge into existing objects.This would fix the issue, however still some work to be done on e2e's. So will leave in draft till then.
cc @acao