keyResolver returns undefined

[tbowmo]

If the keyresolver can't find a match for the key, it returns undefined, resulting in reactQuery throwing up.

Perhaps it should coalesce to null instead?

[tbowmo]

an example

you request ids [1, 5, 7] and backend replies with [{id: 1,...}, {id: 7,...}]

then keyResolver, which uses find, returns undefined for id 5, as it's not able to find it in the backend response. This breaks tanstack/query, which doesn't allow undefined.

Took me a while to find a bug with the above scenario..

[yornaath]

Actually that can easily be handled with a custom resolver:

const batcher = create({
  fetcher: async (ids: number[]) => {
    return fetchUsersByIds(ids);
  },
  resolver: (items, id) => items.find((item) => item.id === id) ?? null,
});

[tbowmo]

Yeah, I know it can be done with a custom resolver, but I also think that the one included in the library should not cause errors when used.

[tbowmo]

You could also just use

const batcher = create({
  fetcher: async (ids: number[]) => {
    return fetchUsersByIds(ids);
  },
  resolver: keyResolver('id') ?? null,
});

The issue is that default behavior can result in undesired side-effects when used with the libraries mentioned in the examples

[yornaath]

No your code isnt correct. keyResolver('id') ?? null will always resolve to the left hand assignment since its a function and not null or undefined. And even though this library is a good fit to use with react-query, its not its responsibility to return something specific that fits its opinionated behaviour.

The responsibility of the resolver is to map the discrete id passed to the individual fetch calls to the batch of returned items. The example I provided of a custom resolver where you need to return null instead of undefined when a id is missing in the returned batch works as intended. Here are some tests to verify:

test("handle undefined responses", async () => {
  const batcher = create({
    fetcher: async (ids: number[]) => {
      return mock.usersByIds(ids);
    },
    resolver: (items, id) => items.find((item) => item.id === id) ?? null,
  });

  const all = await Promise.all([batcher.fetch(2), batcher.fetch(100)]);

  expect(all).toEqual([{ id: 2, name: "Alice" }, null]); // PASSES
});

test("handle undefined responses", async () => {
  const batcher = create({
    fetcher: async (ids: number[]) => {
      return mock.usersByIds(ids);
    },
    resolver: keyResolver('id') ?? null,
  });

  const all = await Promise.all([batcher.fetch(2), batcher.fetch(100)]);

  expect(all).toEqual([{ id: 2, name: "Alice" }, null]); // FAILS
});

Where id 2 is user alice and id 100 does not exist.

[yornaath]

Yeah, I know it can be done with a custom resolver, but I also think that the one included in the library should not cause errors when used.

This is not an issue with this library, that is an up stream issue when consumed with react-query. Returning undefined when they key is not defined in the batch is more in line with js standards where array.find or someObject["missingKey"] is undefined when its not in the array/object, not null.

If you need to massage the data in your upstream consumption of it so that it fits your use that is your responsibillity and can easily be done with a custom resolver or fetch

[tbowmo]

Then could you at least write a note alongside your example in the readme file, about this potential pitfall in relation to tanstack query, now that you got an example for that one?

[tbowmo]

We ended up creating our own keyResolver, that takes a fallback argument, that it returns if the key cannot be found.

[yornaath]

We ended up creating our own keyResolver, that takes a fallback argument, that it returns if the key cannot be found.

Yes this is the correct approach for your use case. Why did you reopen this issue?

[tbowmo]

I still think that the documentation could be better in this regard, so it was an error that I closed the issue

[yornaath]

Just made index and key resolvers return null if they cant find items to make it more out of the box compatible with react-query. Makes sense to me since its the framework this package is mostly used within atm.

🦋  info Publishing "@yornaath/batshit" at "0.10.0"