v1.1.2 Not able to use update() to add entry into many() relation

[heiwen]

In the following example, the final post only has one (1) comment, whereas I would expect it to have two (2).

const postSchema = z.object({
  get comments() {
    return z.array(commentSchema)
  },
})
const commentSchema = z.object({
  text: z.string(),
})

const posts = new Collection({ schema: postSchema })
const comments = new Collection({ schema: commentSchema })

posts.defineRelations(({ many }) => ({
  comments: many(comments),
}))

const post = await posts.create({ 
  comments: [await comments.create({ text: 'First!' })]
});

const comment2 = await comments.create({ text: 'Second!' });

const updatedPost = await posts.update(post, {
  data(p) {
    p.comments = [  ...post.comments, comment2 ];
    // p.comments.push(comment2) also doesn't work
  }})

console.log(updatedPost.comments.length)

I'm using Bun 1.3.2, msw 2.12.2, msw/data 1.1.2 and zod 4.1.12.

[kettanaito]

Hi, @heiwen. Thanks for reporting this. I confirm this is an issue. Probably has to do with the fact that comment2 was created outside of the .update() somehow. Will take a closer look when I have a moment.

[kettanaito]

Discovery

There are two unrelated issues here and they have to do with how post.comments is updated.

Issue 1: .push()

post.comments is a getter that resolves to the actual array:

https://github.com/mswjs/data/blob/ff6721177ddd0f37c60cfb5ee8c3a7468dd683ac/src/relation.ts#L532

Because of that, calling post.comments.push() is not registered by mutative as a mutation (patches is []). Therefore, the update hook is never called and the owner record isn't updated.

I've tried the most obvious approach of wrapping the resolved array in a Proxy, but that makes that array invalid for structuredClone() on which we rely to produce frozenPrevRecord during the update.

Issue 2: Value spreading

Updating post.comments via merge the comments array gets treated differently because there's the "comments" key reassignment, which gets picked up by mutative and the update events are correctly produced. But the resulting comments relation still has incorrect end value after the update.

A potential me not remembering the whole context, but it looks like the update hooks we have do not account for update.nextValue being an array of values:

https://github.com/mswjs/data/blob/ff6721177ddd0f37c60cfb5ee8c3a7468dd683ac/src/relation.ts#L198-L201

An array will produce a false-positive check for !isRecord(update.nextValue), which might lead to all sorts of wonky things. This is likely causing this issue as well. That false positive match means the library thinks we're updating through post (like post.author.id = nextValue) instead of understanding that we're updating the post itself. Value reassignment, although through a relational property, updates the next relation value.

[kettanaito]

I am convinced that the getter characteristic of a relation must be preserved during the update. I.e. we should still pass prevRecord as-is (granted, cleared of internal symbols) to mutative, just like we're doing now:

https://github.com/mswjs/data/blob/ff6721177ddd0f37c60cfb5ee8c3a7468dd683ac/src/collection.ts#L747-L754

This is correct. This ensures that the record you get as an argument in your data function properly resolves relational values and maintains circular relations.

The fact that we're using frozenPrevRecord for the update hook is a bit odd to me but it must have had a reason to be so. I wouldn't be hasty to discard this.

With the getters persisting and mutative not recognizing updates through getters, we have two options of fixing the issue 1:

1. Submit a feature request to mutative to understand updates through getters.
2. Rework the freezing logic so wrapping resolved arrays in proxies would not interfere with anything.

[kettanaito]

I've raised an issue with Mutative (https://github.com/unadlib/mutative/issues/157). Will see what the maintainers think of this.

[heiwen]

@kettanaito Do you think that the fix from unadlib is the right one to merge?

[kettanaito]

@heiwen, I skimmed through it and it seems that it approaches the issue head-on instead of utilizing the concepts we have and rely on in the library. I wouldn't merge it in the shape it is now. It needs a proper review and dissecting it into something we could eventually merge.

It's worth nothing that there are two unrelated issues (like I mentioned above). They might be addressed in different ways.

[kettanaito]

I am on a break from open source until January and will return to this issue at my earliest availability. There are a few more things I wanted to do first as well. I've left enough discovery and technical explanation for this issue here for anyone to jump in and investigate further/propose a solution!