CyCraft / magnetar

A framework-agnostic syncing solution that auto-connects any DB/API with your local data store and has optimistic-UI built in 🌟
https://magnetar.cycraft.co
MIT License
43 stars 5 forks source link

[Bug] undefined document fetch/stream get the whole collection #79

Open vdsbenoit opened 2 years ago

vdsbenoit commented 2 years ago

Hi,

I noticed an odd behavior from magnetar in a edge case. It might not be a bug but then I think it could be highlighted in the documentation 🙂

Use case

I have a method to stream a document that looks like this :

const gamesModule = magnetar.collection<Game>(GAMES_COLLECTION, {
  modifyPayloadOn: { insert: gamesDefaults },
  modifyReadResponseOn: { added: gamesDefaults },
})
export const getGame = (id: string) => {
  const gameModule = gamesModule.doc(id);
  gameModule.stream().catch((error) => {
    console.error(`Game ${id} stream was closed due to an error`, error);
  });
  return gameModule.data;
}

Expected result

Magnetar streams the document with the id id. If id is undefined, nothing happens.

Actual result

If, for some reasons, id is undefined (e.g. when the id is a reactive variable and is not yet set), magnetar streams the whole collection. This behavior led me to massive amount of document streams/fetches on my firestore DB (I reached the 50k free quota in 2h).

Workaround

My current solution was to add a check on the value of the id parameter.

export const getGame = (id: string) => {
  if(!id) return undefined;
  const gameModule = gamesModule.doc(id);
  gameModule.stream().catch((error) => {
    console.error(`Game ${id} stream was closed due to an error`, error);
  })
  return gameModule.data;
}

Suggestion

I think it would be nice if magnetar could handle undefined document ids natively. Else, it would be good to document this behavior.

Again, I am very grateful for the work you have done here! Thanks to magnetar, I was able to write a soon-to-be-published opensource full-stack pwa in ~3 weeks. This app is going to help my non-profit organisation to manage the games and the players of a real-life game that gathers +1200 boyscouts in Belgium next weekend 😉

mesqueeb commented 2 years ago

@vdsbenoit Good catch!!! We should handle this more gracefully and the side effect you found is definitely not what anyone would intend to happen. Open for PRs!

Ps: das cool om te horen! Ik zat ook in de scouts vroeger XD