Hoist merges to top-level

[ricokahler]

Problem

One of the issues with the current API of ReSift is that it's ambiguous which merge function will run when two fetch factories implement a merge from one namespace

For example:

getMovies.js

import { defineFetch } from 'resift';

const makeGetMovies = defineFetch({
  displayName: 'Get Movies',
  share: { namespace: 'movieList' },
  make: /* ... */,
});

const getMovies = makeGetMovies();
export default getMovies;

makeGetMovie.js

import { defineFetch } from 'resift';

const makeGetMovie = defineFetch({
  displayName: 'Get Movie',
  share: {
    namespace: 'movieItem',
    merge: {
      // this is merge 1 👇
      movieList: (prevMovie, nextMovieList) => /* ... */,
      // 👆👆👆
    },
  },
  make: /* ... */,
});

makeUpdateMovie.js

import { defineFetch } from 'resift';

const makeUpdateMovie = defineFetch({
  displayName: 'Update Movie',
  share: {
    namespace: 'movieItem',
    merge: {
      // this is merge 2 👇
      movieList: (prevMovie, nextMovieList) => /* ... */,
     // 👆👆👆
    },
  },
});

---

So in this situation, there are 2 namespaces: movieList and movieItem and there are 3 fetch factories: getMovies, makeGetMovie, and makeUpdateMovie.

The ambiguity comes from the following question: when a SUCCESS occurs the movieList namespace (i.e. the getMovies fetch factory), which merge ("merge 1" or "merge 2") is ran?

---

The answer to this question is actually which ever merge was associated with a fetch factory that was dispatch last — a convoluted and weird answer.

Proposal

Instead of having the merges in the fetch factories, I propose we hoist them up to the <ResiftProvider> using a merges property. The convention would be to create a new file or folder called merges that will handle doing the merging from all namespaces to other namespaces.

index.js

import React from 'react';
import { render } from 'react-dom';
import { ResiftProvider } from 'resift';
import App from './App';

import dataService from './dataService';
import merges from './merges';

const container = document.querySelector('#root');

render(
  <ResiftProvider dataService={dataService} merges={merges}>
    <App />
  </ResiftProvider>,
  container
);

merges.js

const merges = {
  incomingNamespace: {
    destinationNamespace: /* ... mergeFunction ... */,
  },

  movieList: {
    movieItem: (previousMovieItem, nextMovieList) => {
      // return a new movieItem using the `nextMovieList`
    }
  },
};

export default merges;

The proposal is simple: instead of having the merges on the fetch factory, hoist them to the provider.

This will actually make the code much more simple. We can remove a lot of confusing logic in the reducers.

ReSift/src/dataServiceReducer/sharedReducer.js

Lines 23 to 31 in d0b6a1f

	const mergeState = { ...state?.merges };
	// (eslint bug)
	// eslint-disable-next-line no-unused-vars
	for (const [targetNamespace, mergeFn] of Object.entries(mergeObj)) {
	const merges = { ...mergeState?.[targetNamespace] };
	merges[namespace] = mergeFn;
	mergeState[targetNamespace] = merges;
	}

👆this code snippet is creating a lookup table of merge objects on FETCH so that it can correctly know how to merge in an incoming SUCCESS. If that sounds convoluted, it's because it is 😅.

If we hoist the merges to the provider level then we can deprecate this code and make things a bit more straightforward.

Summary

Problem: It's ambiguous which merge function will run when there is more than one merge function for the same source-target pairing of namespaces.
Solution/Proposal: Hoist the merges definition to the provider.
API Changes: share.merge will be deprecated (or possibly removed if possible) in favor of adding a merges prop to the <ResiftProvider> component.

[pearlzhuzeng]

I like this proposal a lot! I think it provides clarity to both the developer and the ReSift reducer.