Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow more specification to the loading status behavior #39

Open
ricokahler opened this issue Nov 22, 2019 · 2 comments
Open

Allow more specification to the loading status behavior #39

ricokahler opened this issue Nov 22, 2019 · 2 comments
Assignees
Labels
proposal This contains a proposal for a feature or fix. This work may be subject to be discarded.
Milestone

Comments

@ricokahler
Copy link
Contributor

@pearlzhuzeng had experienced an unexpected result regarding the LOADING status when developing the ReSift Rentals demo.

The demo is application is structured like Netflix's list of lists of movies. She had a few fetches that we were shared and they were also using the newer "merges across namespaces" feature.

The issue and question is: what is the status of a fetch that has a share across a namespace?

Here's the scenario, you have three fetches with two different namespaces:

  • makeGetGenre - a fetch factory that gets a list of movies. namespace: genre
  • makeGetMovie - a fetch factory that gets a single movie. namespace: movie
  • makeUpdateMovie - a fetch factory that updates a single movie. namespace: movie

makeGetGenre implements a merge for the namespace movie so that when a movie item changes, the genre list changes to reflect that change.

The question is, what is the status of a genre when a movie is inflight?

// after you dispatch an update...
const updateMovie = makeUpdateMovie(movieId);
dispatch(updateMovie());
// ...what is the status of the genre list?
const getGenre = makeGetGenre(genreId);
const status = useStatus(getGenre);

// what should this be??
console.log(isLoading(status));

Through the current implementation, the output of isLoading is true.

With this behavior, whenever any movie goes inflight, it will cause all lists to have the status LOADING. This is because the genre namespace doesn't know which movie belongs to which genre (and you can't know ahead of time).

This can be confusing to the developer because an update to a movie in a different genre would cause all the genres to update (which is what @pearlzhuzeng experienced). I think this behavior should not be the default and we should add other behavior options.


The proposal is to remove the option isolatedStatus and replace it with type.

The possible types of values of type will be:

  • self - only consider the status of the current fetch factory + key
  • namespace - only consider the statuses of all fetches related the current namespace + key
  • all - consider the statuses of all the fetches related to the current namespace + key as well as any other namespace (e.g. genre namespace and movie).
const selfStatus = useStatus(getGenre, { type: 'self' }); // this is the new `isolatedStatus: true`
const namespaceStatus = useStatus(getGenre, { type: 'namespace' });
const allStatus = useStatus(getGenre, { type: 'all' });

I think the default type for useStatus should be namespace.


That's it! Let me know if you need any further clarification. I think @pearlzhuzeng knows what I'm talking about though.

@ricokahler ricokahler added the proposal This contains a proposal for a feature or fix. This work may be subject to be discarded. label Nov 22, 2019
@JulesPatry
Copy link
Contributor

Interesting concept. I think namespace is what separates resift from other data management libraries.

@ricokahler ricokahler added this to the v0.2.0 milestone Dec 3, 2019
@ricokahler ricokahler changed the title Loading status behavior Allow more specification to the loading status behavior Dec 3, 2019
@pearlzhuzeng
Copy link
Member

pearlzhuzeng commented Jan 28, 2020

@ricokahler Thank you for the proposal write up!

Personally, I feel that the fetch status for getGenre should be loading if it needs to merge in the change of getMovie whenever a movie within that genre gets updated.

The challenge is that getGenre doesn't know which genre that updated movie belongs to.

So I think figuring out a way to inform getGenre the genreId of that updated movie would be a better route to go. The most straightforward way may be updating makeGetGenre to expose the genreId of that fetch instance in the return of the fetch. Since when we fetch a movie with getMovie, we are also getting a genres array along with the movie object that indicates the genres that movie belongs to, then in the merge block in getGenre, we can check if the the genre id of the intended fetch is included within the genre id array of the movie before committing a merge. Please take a look at the example below, especially the two lines marked by '👈'

const makeGenre = defineFetch({
  displayName: 'genre',
  share: {
    namespace: 'moviesOfGenre',
    merge: {
      moviesOfGenre: (previous, response) => {
        ...
        };
      },
      movie: (previousMovies, incomingMovie) => {
        if (!previousMovies) return null;
        
        if (!incomingMovie.genreIds.includes(previousMovies.genreId)) return null; 👈

        const index = previousMovies.response.results.findIndex(movie => movie.id === incomingMovie.id);

        if (index === -1) {
          return previousMovies;
        }

        return {
          ...previousMovies,
          response: {
           ...previousMovies.response, 
            results: [
              ...previousMovies.results.slice(0, index),
              {
                ...previousMovies.results[index],
                name: incomingMovie.name,
              },
              ...previousMovies.results.slice(index + 1, previousMovies.results.length),
            ],
        }};
      },
    },
  },
  make: genreId => ({
    request: page => async ({ http }) =>
      { 
        const response = await http({
          method: 'GET',
          route: `/genres/${genreId}/movies`,
          query: {
            page,
            pageSize: 10,
          },
        });

        return { response, genreId } 👈
      }
  }),
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal This contains a proposal for a feature or fix. This work may be subject to be discarded.
Projects
None yet
Development

No branches or pull requests

3 participants