igorkamyshev / farfetched

The advanced data fetching tool for web applications
https://ff.effector.dev
MIT License
186 stars 34 forks source link

[API Design](Cache): Should ignore passed params into query for generating cache key if not expected by query #349

Open NazariiShvets opened 1 year ago

NazariiShvets commented 1 year ago

Precondition: docs

So, the key is a hash of the following data:
- SID of the Query
- params of the particular call of the Query
- current values of all external Stores that affect Query
To get short and unique key, we stringify all data, concatenate it and then hash it with SHA-1.

This design desigion does not take into account pattern within effector community with sample: If EventPayload of target is void, user can pass data in any shape without any warning, thus it can be unintuitive, why they hit cache miss on, for example, page route params refetch

const getDataQuery = createQuery({
  effect: () => Promise.resolve([])
})

sample({
   clock: pageOpened // Event<{ params: { id: '1' }, query: {} }>
   target: getDataQuery.start
})

sample({
   clock: pageUpdated // Event<{ params: { id: '2'}, query: {} }>
   target: getDataQuery.start
})
igorkamyshev commented 12 months ago

It works for createJsonQuery, so I think we should back port this feature to creteQuery as well.

igorkamyshev commented 6 months ago

So, it's more complicated 🤔

What if we replace the handler of a Query during fork?

const q = createQuery({
  effect: () => Promise.resolve([])
})

const scope = fork({
  handlers:[[q.__.executeFx, (p) => return [p]]]
})

We cannot skip params in this case, it would be harmful. So, we have to check usage of params in runtime.

I suggest you just remove params in sample by fn. Something like this 👇

const getDataQuery = createQuery({
  effect: () => Promise.resolve([])
})

sample({
   clock: pageOpened // Event<{ params: { id: '1' }, query: {} }>
   fn: () => null,
   target: getDataQuery.start
})

sample({
   clock: pageUpdated // Event<{ params: { id: '2'}, query: {} }>
   fn: () => null,
   target: getDataQuery.start
})

We will think about improving DX in this case later 🙏