How to resolve getData in sequence

[ignatevdev]

Hi. I'm having a trouble trying to figure out how to make my data request to fire in sequence.
In particular, I have a following config

<Route
    Component={App}
    path="/"
    getData={App.getData}
>
    <Route
        path="pages/:id"
        Component={Page}
        getData={Page.getData}
        render={authorizedOnly}
    >
        <Route
            path="settings"
            getData={Settings.getData}
            Component={Settings}
        />
    </Route>
</Route>

I would like my root getData to fetch the authorization status and make the nested routes wait until the promise is resolved. Then, after auth is resolved, I want to check my authorization status, and if the user is logged in /pages/:id route will load the page's data. And eventually, if Page's getData didn't throw a 404 error, /pages/:id/settings promise should fire and load the data depending on the page's type.

Now all promises fire at once and don't wait until either of them finishes, and as far as I understand this is an intended logic. However, I need a way to make nested getData calls to be pending until upper-level getData promises are resolved, and in case if upper-level promises throw a redirect or 404 error I don't want lower-level getData to fire at all.

Is this possible with found?

Thanks

[taion]

Are you using some central store like Redux?

The way you would do this is to have some sort of app-level cache or store. In the child getData, wait on some promise for the parent data to be available. Then, once those data are available, the child getData can use those results.

This is easiest if you have some sort of primitive that's like "retrieve this piece of data if it's not already in the cache or has a pending request". But it is guaranteed that your getData calls will fire in order from parent to child, so you could just check for a promise somewhere.

[ignatevdev]

Yes, I am using redux and I've thought about this, but this seems a bit dirty, because this basically means that I have to implement the pending state myself and copy it across every component. For example, in every getData of my app I would need to check if my auth state is still resolving and wait until it's finished. This would work, of course, but did you consider to add a concept of deferred routes, which would wait until the parent component's promises are resolved?

I could probably make a PR for this if I'll have enough time

[taion]

We thought about it a bit, but we ultimately didn't end up with cases where that was necessary, especially because we mostly use Relay, which is less friendly to that sort of pattern.

It's a bit of a trade-off either way, I guess. If you explicitly state dependencies in getData, then you get more control, so if you add an extra parent route with async data dependencies, then you don't necessarily delay child getData calls, the way you would if you just had a deferred flag. If we can figure out an API that makes sense, then I'd be happy to accept a PR. We could also make it a different resolver (it's supposed to be pluggable anyway).

If it were just a single async check for e.g. authorization, you could do something like:

class AuthorizedDataRoute extends Route {
  constructor({ getDataWithAuthorization, ...props }) {
    super({
      ...props,
      getData: (match) => (
        getAuthorizationData(match).then(authorizationData => (
          getDataWithAuthorization(match, authorizationData)
        )
      ),
    });
  }
}

If you want something quick and dirty, and you have caching, you could also just loop through your route config object and recursively set up getData promises, something like:

function setDeferredGetData(routeConfig, pathGetData = () => {}) {
  for (const route of routeConfig) {
    if (!route.deferred && !route.children) {
      continue;
    }

    const originalGetData = route.getData;
    let routeGetData;
    
    if (originalGetData) {
      routeGetData = (match) => (
        Promise.resolve(pathGetData(match)).then(
          () => originalGetData.call(route, match),
        )
      );
    } else {
      routeGetData = pathGetData;
    }
    
    if (route.deferred) {
      route.getData = routeGetData;
    }
    
    if (route.children) {
      setDeferredGetData(route.children, routeGetData);
    }
  }
}

[ignatevdev]

I already figured out a small hack which I can use to solve the issue, but I think that other people may also meet this problem and I would like to add an option to the library which would defer the route's promises automatically.

The API would be simple - user will provide a boolean defer prop on the , and a Route with this flag set to true will not start resolving until all it's parent's Routes have finished resolving.

If you agree with this proposal, it would be nice if you could point me in a direction of the best way to implement it, since I'm not familiar with found's codebase.

[taion]

Just so I understand, the goal here is that you don't want the nested getData fetching to happen, in case the parent routes throw a "not found" or a redirect, but otherwise you don't have explicit data dependencies?

That sounds like a reasonable use case.

You'd want to start with

found/src/resolver.js

Lines 17 to 21 in b9a57cd

	const data = getRouteValues(
	routeMatches,
	route => route.getData,
	route => route.data,
	);

. I need to extract a helper to traverse through the routes in an order that respects nested children, though, but I need to do that anyway. The idea on your side would be, though, that you replace getRouteValues with something more specific that maybe builds an array of the parent getData promises, and adds a Promise.all before calling any child getData bits.

Let me know how that sounds. I can pull out that traversal helper in the next few days.

[ignatevdev]

you don't want the nested getData fetching to happen, in case the parent routes throw a "not found" or a redirect

Yes, exactly, but also in some cases I have some explicit data dependencies which I will have to load for all nested routes beforehand, but the API would be the same anyway, so it doesn't matter too much.

Making a function which builds a promise sequence depending on routes sounds reasonable for me, so as soon as you will roll out the helper I will be able to start working on it

[ignatevdev]

@taion I've looked at the found's code and gathered an understanding of how to implement this. The helper that you've mentioned would be really useful, so could you tell me when could you roll it out?

[taion]

Soon! Sorry, it’s been really busy this week.

[ignatevdev]

No problem, just let me now when it's ready, I will be trying to make some tests and prototype implementations for now

[ignatevdev]

I've come up with this small fix. Since getRouteValues iterates over routes in order of depth, I save the parallel promises into the promises array and return the promise as it would do normally. But when it encounters a route with a deferred flag set to true, instead of executing the getData immediately, it makes it execute only after all previous promises have resolved and clears the array allowing for nested deferred routes.

This approach prevents the getData calls if any of the parent promises fail, so if 404 error was thrown, no unnecessary requests would execute, and it also allows for ensuring the data dependencies presence for nested routes.

With these modifications all the current tests pass and it works great in my project. What do you think?

export function getRouteValues(routeMatches, getGetter, getValue) {
  let promises = [];

  return routeMatches.map(match => {
    const { route } = match;
    const getter = getGetter(route);

    if (getter) {
      if (route.deferred) {
        const promise = Promise.all(promises).then(() =>
          getter.call(route, match),
        );

        promises = [promise];

        return promise;
      }

      const promise = getter.call(route, match);

      promises.push(promise);

      return promise;
    }

    return getValue(route);
  });
}

[taion]

That looks about right. I'll get the helper up soon.

[taion]

It'd be a separate getRouteData, though – we use getRouteValues for other things, so we wouldn't want to change that.

[ignatevdev]

It's not a problem, I could make a PR with this change any time and maybe a couple of tests if needed

[taion]

See #165.

We may want to make this a different resolver, though, since this will require creating temporary objects.

I guess alternatively we can scan through the matches and check if any route has deferred set. This could make more sense as a separate resolver though.

[ignatevdev]

Hm, I'm not sure if it needs to be a separate resolver, because by default it doesn't change the current behaviour in any way and a separate resolver wouldn't have any differences with the original besides the getRouteValues replacement, so basically it would be a copypaste.

But it doesn't matter for me, just tell me which way should I do it and I will make a PR in a couple of days

[taion]

Okay, let's just add it here, then.

[taion]

If you have a chance, though, could you let me know whether #165 looks right to you? I might cut a quick release for that just to unblock my long-standing Found Relay issue.

[ignatevdev]

Looks fine to me, but I did not understand entirely - should I use accumulateRouteValues to generate the data array in resolver or should I stick the the current getRouteValues implementation? I can't see the difference

[taion]

You'd need to do both. The reason behind accumulateRouteValues is that, if you use named child routes, then your match is not a list but instead a tree, and accumulateRouteValues makes sure that the value you use for accumulating only reflects your ancestors in the tree.

For example, the route tree might look like:

        C
      /
A - B 
      \
        D

As a list, we might represent this as [A, B, C, D], but if you're trying to process something for D, you don't want it to see the results for C, because C is not a parent of D – only A and B are.

[ignatevdev]

Got it. There is one more question though - which tests do I have to make (if any) for the deferred routes?

[taion]

I don't think I have any test coverage at all for this resolver \:

If you can add some basic test coverage, great. If not, I'm fine without additional test coverage for now, assuming you can check that it does in fact work for you.

[ignatevdev]

@taion Hi again! It's been a long time since I've had my hands on this feature, but now I finally have the time to finish it and after some struggling I have come up with a function, which works as we have discussed. However, I'm not confident if my solution is right, because it feels a bit sketchy, therefore, I post the code of the function here before opening a PR and would really appreciate to hear your comments on this.

export function getRouteData(routeMatches, getGetter, getValue) {
  const promises = {};

  return routeMatches.map(match => {
    const getter = getGetter(match.route);

    if (getter) {
      let currentValue;

      // Go through all the ancestors and gather info about their promises
      accumulateRouteValues(
        routeMatches,
        match.routeIndices,
        (value, { route }) => {
          const path = `${value.path}/${route.path}`;

          // Get the pending promise if exists
          let promise = promises[path];

          // If the callback should be deferred and has not been executed
          // get the last ancestor promise to hook on to
          if (route.defer && !promise) {
            promise = value.lastPromise;
          } else if (value.lastDeferredPromise) {
            // If one of the ancestors is deferred, delay the child callbacks until it has resolved
            promise = value.lastDeferredPromise;
          }

          // Generate the route data
          const routeValue = {
            path,
            lastDeferredPromise:
              (route.defer && promise) || value.lastDeferredPromise,
            lastPromise: promise || value.promise,
            promise,
          };

          // Get the current route value
          if (match.route === route) {
            currentValue = routeValue;
          }

          // Continue accumulation
          return routeValue;
        },
        {
          path: '',
        },
      );

      if (getter) {
        // If there is a promise in the currentValue, we should execute getter only after it resolves
        const result = currentValue.promise
          ? currentValue.promise.then(() => getter.call(match.route, match))
          : getter.call(match.route, match);

        // Check if getter has returned a promise and save it
        if (isPromise(result)) {
          promises[currentValue.path] = result;
        }

        return result;
      }
    }

    return getValue(match.route);
  });
}

This function replaces getRouteValues in resolver and getComponents

[taion]

Sorry for the delayed response.

That's mostly right. I don't quite understand why you're storing path, though – why do you need anything beyond the last value for getting the promise?

[ignatevdev]

Because I need to save the promise in the promises object in order to get it later on and chain another promise on it. For that purpose I need to have and an id of the route, and I didn't find any better id than it's full path. Saving by the route path would not work, because it can be the same for two routes. If you know a better way to do this, I'm willing to fix this

[taion]

Why isn't the value in the callback arg sufficient, though?

[ignatevdev]

Did not quite understand you on this. Sufficient for what?

If you were asking why it's not sufficient for building the promise chain, then the answer is because accumulateRouteValues has to be called on each match, but the callbacks for the ancestor routes have to be executed only once. If you would try to build the promise chain for every match without caching the promises, it would simply create duplicate calls and requests, which is not what we want.

If the question is why not use the value as the id, then you can see, that the value is an object, and I'm not quite sure how it would work as the key in object or a Map

[taion]

accumulateRouteValues only calls the callback once for each matched route. If you have a route match tree that looks like:

        C
      /
A - B 
      \
        D

It will call for:

• A, with initial value
• B, with value from A
• C, with value from B
• D, with value from B

So even for ancestors it will only run once.

[ignatevdev]

Of course, but currently I call it for each match inside the map function, so it would run all the callbacks multiple times for each match. If you propose to move the accumulateRouteValues outside the map, generate the required sequence and then map it to the matches array, that'd propably do, but there are two reasons why I haven't tried that.

First reason is that accumulateRouteValues expects an array of routeIndices, which is accessible only from an individual match. Since we get an array of matches, I assumed that I need to run it for each match and use their individual routeIndices.

And the second reason is that I don't quite understand how we would then map accumulated values back to the matches array in your example from the last message. Would the order of named route values be always the same as in routeMatches array?

[taion]

oh, shoot – i missed that. but, yeah, you can just use routeMatches[0].routeIndices. they're guaranteed to be the same for every element in that list. i hadn't thought of that use case.

and the other guarantee that accumulateRouteValues makes is that it will in fact return an array in the same order, so you can just return its return value directly

[taion]

by

i hadn't thought of that use case.

i mean specifically that what i have above works... it's just that the API is slightly awkward since you have this array where you're only looking at the first value...

but in this case i think that's fine. at least, i think a comment explaining something like "routeMatch.routeIndices is the same for every route match" would be sufficient

[ignatevdev]

Alright, I'll try to rewrite this piece in the next few days then and report it back here.
Thanks!

[taion]

thanks! feel free to put anything you have in a PR – i think it's easier to use PR line comments anyway

[ignatevdev]

Got it, next time I'll come back with a PR then ;)