vuejs / rfcs

RFCs for substantial changes / feature additions to Vue core
4.87k stars 546 forks source link

New Async Component API #148

Closed yyx990803 closed 4 years ago

yyx990803 commented 4 years ago
import { defineAsyncComponent } from "vue"

// simple usage
const AsyncFoo = defineAsyncComponent(() => import("./Foo.vue"))

// with options
const AsyncFooWithOptions = defineAsyncComponent({
  loader: () => import("./Foo.vue"),
  loadingComponent: LoadingComponent,
  errorComponent: ErrorComponent,
  delay: 200,
  timeout: 3000
})

Full Rendered Proposal

cexbrayat commented 4 years ago

After playing a bit with it (now that it is released in 3.0.0-alpha.10), I just want to point out that it feels a bit weird, for TS users, to have defineComponent and createAsyncComponent, instead of defineAsyncComponent.

As @yyx990803 pointed out defineComponent doesn't do anything itself, whereas createAsyncComponent creates an async wrapper. But I think it would be easier to learn/find if we have a coherent API:

import { defineAsyncComponent, defineComponent } from 'vue';

export default defineComponent({
  name: 'App',
  components: {
    PendingRaces: defineAsyncComponent(() => import('./PendingRaces.vue'))
  },
});
CyberAP commented 4 years ago

I find it rather strange that we have some API's that include action in its name and some don't. For example reactive is short for createReactiveObject or just createReactive, but createAsyncComponent doesn't fit that pattern. What about calling it just asyncComponent?

CyberAP commented 4 years ago

I would also like to know how we can restart async component loader in case it fails? Probably something like a retry option or even more preferably a programmatic control over the failed behaviour? (ability to place a Reload button on the Error component would be much appreciated)

What about delegating it to Suspense?

<template>
  <Suspense @error="onSuspenseError" ref="suspense">
    <MyAsyncComponent />
  </Suspense>
</template>

<script>
  export default {
    methods: {
      onSuspenseError(asyncComponent, error) {
        this.$refs.suspense.retry(asyncComponent);
        // or
        asyncComponent.retry();
      }
    }
  }
</script>

How do we pass promise rejection result to an error component?

CyberAP commented 4 years ago

How refs should be used with async components in options and composition API?

yyx990803 commented 4 years ago

@cexbrayat yeah, on a second thought, it does make sense to make them consistent. Would you mind re-doing the PR?

cexbrayat commented 4 years ago

@yyx990803 Sure, here it is https://github.com/vuejs/vue-next/pull/888 Thank you for considering it.

yyx990803 commented 4 years ago

@CyberAP reactivity APIs are used in much higher frequency so we are sacrificing a bit of explicitness for readability. Component declarations do not have this problem.

Re retry - what about an onError option?

const Foo = defineAsyncComponent({
  loader: () => import('./Foo.vue'),
  onError: (error, retry) => {
    // decide whether to retry based on error...
    // if retry() is called, it will not go into error state (stay on loading state)
  },
  error: ({ error, retry }) => {
    // the error component also receives the retry function via props so you can
    // let the user decide whether to retry
    return h('button', { onClick: retry }, 'retry')
  }
})

As for retry with Suspense - I think it can be considered a feature addition that isn't going to cause conflicts with the current design. In general, the whole Suspense API could use more polishing (it will be labeled as experimental for initial 3.0 release), for example how to replicate the delay / error handling behavior of 2.x async components with Suspense. I think we should make it a separate RFC.

CyberAP commented 4 years ago

Re retry - what about an onError option?

To me that looks a bit confusing since it's hard to tell what triggers what. Is it onError that calls error, or is it error that then calls onError.

What if the loader could receive these callbacks?

const Foo = defineAsyncComponent({
  loader: (error, retry) => {
    return import('./Foo.vue')
      .catch(res => {
        if (res.code) return retry();
        error();
      });
  },
})

If the return result in loader is a rejected promise then error is called automatically. But I am concerned that error and retry only can be too limiting, there may be cases when we don't want any of these and for example want to mess with async component's client context in some way or another.

error: ({ error, retry })

This to me is also a bit confusing, especially that an option and an argument (even destructured) share the same name. For beginners it might not be obvious which closure is used for error within the callback.

yyx990803 commented 4 years ago

@CyberAP passing retry directly to loader is a good idea. Although I don't think the error argument is needed since you can just throw the caught error:

const Foo = defineAsyncComponent({
  loader: (retry) => {
    return import('./Foo.vue')
      .catch(error => {
        if (error.code) {
          return retry()
        } else {
          throw error
        }
      })
  },
})

Re error: ({ error, retry }) => {} - it's actually props passed to the (functional) component.

leopiccionia commented 4 years ago

Does it suppose that only one retry is allowed? Or could it be configurable somehow?

If configurable, I'd prefer it if that was an option to defineAsyncComponent (e.g. maxRetries: 3), to avoid this kind of error-prone boilerplate:

function loadComponent (path, maxRetries = 1) {
  let retriesCount = 0

  return (retry) => {
    retriesCount++

    return import(path).catch((error) => {
      if (error.code && retriesCount <= maxRetries) {
        retry()
      } else {
        throw error
      }
    })
  }
}
yyx990803 commented 4 years ago

@leopiccionia doable, but the downside is when the max retries have been reached, Vue wouldn't be able to report the original error unless you pass it to the retry call, so the usage will have to look like:

const Foo = defineAsyncComponent({
  loader: retry => import('./Foo.vue').catch(err => {
    if (err.code) return retry(err)
    throw err
  }),
  maxRetries: 3
})

You will need to remember to

  1. Return the retry call (otherwise it won't work)
  2. pass the original error to retry (so that when max retries are reached Vue can handle the original error)

It's still quite a bit of boilerplate and potential pitfalls.

On the other hand, instead of manually catching and retrying, the only part you really want to write is deciding whether to retry based on the caught error, so maybe this is easier to use:

const Foo = defineAsyncComponent({
  loader: () => import('./Foo.vue'),
  retryWhen: error => error.code === 123,
  maxRetries: 3
})
yyx990803 commented 4 years ago

This RFC is now in final comments stage. An RFC in final comments stage means that:

The core team has reviewed the feedback and reached consensus about the general direction of the RFC and believe that this RFC is a worthwhile addition to the framework. Final comments stage does not mean the RFC's design details are final - we may still tweak the details as we implement it and discover new technical insights or constraints. It may even be further adjusted based on user feedback after it lands in an alpha/beta release. If no major objections with solid supporting arguments have been presented after a week, the RFC will be merged and become an active RFC.

Morgul commented 4 years ago

On the other hand, instead of manually catching and retrying, the only part you really want to write is deciding whether to retry based on the caught error, so maybe this is easier to use:

const Foo = defineAsyncComponent({
  loader: () => import('./Foo.vue'),
  retryWhen: error => error.code === 123,
  maxRetries: 3
})

I agree that retryWhen is a lot more usable; my concern is that baking in retry options opens up a lot of surface area to this api. What if you want a specific backoff strategy? Dynamic number of retries based on the error response? The ability to track the number of retries (for logging or other purposes)?

I really like the simplicity of the retry function, especially it being passed to loader. I'd hate to lose that flexibility. Is there a way we could provide something that covers, say, 80% of people's use cases (because we don't want to encourage boilerplate) but also keep the power of the retry function?

Would providing a default retryStrategy implementation be out of the question? Something like:

import { retryStrategy } from "vue"

const Foo = defineAsyncComponent({
  loader: () => import('./Foo.vue'),
  retryStrategy: retryStrategy({
    retryWhen: error => error.code === 123,
    maxRetries: 3
  })
})
CyberAP commented 4 years ago

In a case where we'd like to handle errors inside loader the only way of doing so is to throw the error again so retryWhen receives it.

const Foo = defineAsyncComponent({
  loader: () => import('./Foo.vue').catch(error => {
    handleError(error);
    throw error;
  }),
  retryWhen: error => error.code === 123,
  maxRetries: 3
})

With a callback argument that could look like this:

const Foo = defineAsyncComponent({
  loader: (fail, retry, tries) => import('./Foo.vue').catch(error => {
    handleError(error);
    if (tries <= 3 && error.code === 123) { retry(); }
    else { fail(); }
  }),
})
yyx990803 commented 4 years ago

Errors thrown in the loader can be handled by standard Vue error handling mechanisms (errorCaptured and global config.errorHandler). Using the callback, on the other hand, makes it impossible for Vue to even be aware of that error unless you remember to pass the error to retry and fail. So I don't think manual catch is the way to go. A more flexible retry option could be considered.

backbone87 commented 4 years ago

imho the onError is the simplest solution, which informs vue & allows functional style:

const Foo = defineAsyncComponent({
  loader: () => import('./Foo.vue'),
  onError: ({ error, fail, retry, attempts }) => {},
});

const ON_ERROR_DEFAULT = ({ error, fail }) => {
  console.log(error);
  fail(error);
};

function retry(maxAttempts) {
  return ({ error, fail, retry, attempts }) => {
    if (attempts < maxAttempts) {
      return void retry();
    }

    console.log(error);
    fail(error);
  }
}

const Foo = defineAsyncComponent({
  loader: () => import('./Foo.vue'),
  onError: retry(3),
});

edit:

function retryWhen(condition) {
  return ({ error, fail, retry, attempts }) => {
    if (condition({ error, attempts })) {
      return void retry();
    }

    console.log(error);
    fail(error);
  }
}

const Foo = defineAsyncComponent({
  loader: () => import('./Foo.vue'),
  onError: retryWhen(({ error, attempts }) => Math.random() > 0.5),
});
CyberAP commented 4 years ago

Errors thrown in the loader can be handled by standard Vue error handling mechanisms (errorCaptured and global config.errorHandler). Using the callback, on the other hand, makes it impossible for Vue to even be aware of that error unless you remember to pass the error to retry and fail. So I don't think manual catch is the way to go. A more flexible retry option could be considered.

That makes sense and I get the idea of having as little verbosity and complexity as possible, but at the same time throwing errors within a catch block in order to launch retry is still not optimal in my opinion, primarily because it's implicit in its intent.

What about moving catch block to a constructor option? And accept a boolean return from that callback to decide whether to retry or not? That way error is still proxied by Vue and can be handled by the user.

const Foo = defineAsyncComponent({
  loader: () => import('./Foo.vue'),
  catch: ({ error, tries }) => {
    handleError(error);
    return (error.code !== 500 || tries <= 3);
  }
})

Or in a case we don't want to handle an error:

const Foo = defineAsyncComponent({
  loader: () => import('./Foo.vue'),
  catch: ({ tries }) => tries <= 3,
})
yyx990803 commented 4 years ago

@CyberAP that doesn't seem to be much different from retryWhen, and it doesn't have the flexibility @Morgul wanted.

I agree with @backbone87 that a separate onError option might be the most straightforward.

Morgul commented 4 years ago

@CyberAP that doesn't seem to be much different from retryWhen, and it doesn't have the flexibility @Morgul wanted.

I agree with @backbond87 that a separate onError option might be the most straightforward.

I do really like onError, and I think it's the best set of compromises. I don't like that there'll be onError and error as @CyberAP said, but documentation should be able to clarify the difference. I think it's the right api choice, structurally, at any rate.

yyx990803 commented 4 years ago

@Morgul error has been renamed to errorComponent.

backbone87 commented 4 years ago

i would like to make another proposal:

type Loader = () => Promise<Component | EsModuleComponent>;

// simple variant
// - loads once
// - uses browser timeout for requests
// - uses errorComponent on rejection
defineAsyncComponent({
  loader: () => import("./Foo.vue"),
  errorComponent: ErrorComponent,
  loadingComponent: LoadingComponent,
  delay: 200
})

// using timeout helper
function timeout(t: number, loader: Loader): Loader {
  return () => Promise.all(rejectAfter(t), loader());
}
defineAsyncComponent({
  loader: timeout(3000, () => import("./Foo.vue")),
  errorComponent: ErrorComponent,
  loadingComponent: LoadingComponent,
  delay: 200
})

// using retryWhen helper
type Condition = (e: unknown, attempt: number) => boolean;
function retryWhen(condition, loader: Loader): Loader {
  return async () => {
    let attempt = 0;
    while(++attempt) {
      try {
        return await loader();
      } catch (e) {
        console.log(e);
        if (!condition(e, attempt)) {
          throw e;
        }
      }
    }
  }
}
defineAsyncComponent({
  loader: retryWhen((e, attempt) => Math.random() * attempt > 0.5, () => import("./Foo.vue")),
  loadingComponent: LoadingComponent,
  errorComponent: ErrorComponent,
  delay: 200
})

// using retry helper
function retry(attempts, loader: Loader): Loader {
  return () => retryWhen((e, attempt) => attempt <= attempts, loader);
}
defineAsyncComponent({
  loader: retry(3, () => import("./Foo.vue")),
  loadingComponent: LoadingComponent,
  errorComponent: ErrorComponent,
  delay: 200
})

// using the fallback helper
function fallback(...loaders: Loader[]): Loader {
  return async () => {
    let lastError = new Error();
    for (const loader of loaders) {
      try {
        return await loader();
      } catch (e) {
        console.log(e);
        lastError = e;
      }
    }
    throw lastError;
  }
}
defineAsyncComponent({
  loader: fallback(() => import("./Foo.vue"), () => import("./Bar.vue"), () => import("./Baz.vue")),
  loadingComponent: LoadingComponent,
  errorComponent: ErrorComponent,
  delay: 200
})

// combining helpers
defineAsyncComponent({
  loader: timeout(10000, fallback(timeout(1000, () => import("./Foo.vue")), retry(3, () => import("./Bar.vue")), () => import("./Baz.vue"))),
  loadingComponent: LoadingComponent,
  errorComponent: ErrorComponent,
  delay: 200
})

// custom defineAsyncComponent
function myDefineAsyncComponent(loader: Loader, t = 10000, attempts = 3): AsyncComponent {
  return defineAsyncComponent({
    loader: retry(attempts, timeout(t, loader)),
    loadingComponent: LoadingComponent,
    errorComponent: ErrorComponent,
    delay: 200
  })
}
myDefineAsyncComponent(() => import("./Foo.vue"));

// even errorComponent & loadingComponent can be defined in terms of a custom loader
function errorComponent(loader: Loader, comp: Component): Loader {
  return loader().catch(() => comp);
}
function makeLoadingProxy(loading: Promise<Component>, comp: Component): () => FunctionalComponent {
  return () => {
    const target = ref(comp);
    loading.then((c) => void (target.value = c));

    return (props, { slots }) => h(target.value, props, slots);
  }
}
function loadingComponent(loader: Loader, comp: Component, delay: number): Loader {
  return () => {
    const loading = loader();

    return Promise.race(
      loading,
      resolveAfter(delay).then(makeLoadingProxy(loading, comp))
    );
  };
}
yyx990803 commented 4 years ago

@backbone87 I think this is a bit too convoluted for typical usage, and most of it can be done in userland. If you prefer such a style you can surely do that, but I don't think it would be a good fit for average users.

backbone87 commented 4 years ago

@yyx990803 yes! and that occured to me only after i wrote this. the conclusion i would draw from this:

edit: the most basic atom would be something like this:

function defineLoadedComponent(loader: Loader): FunctionalComponent {
  // maybe memoize?
  // loader = memoize(loader);

  return async (props, { slots }) => h(await loader(), props, slots);
}
yyx990803 commented 4 years ago

@backbone87 internally defineAsyncComponent creates a wrapper component managing the loading state using standard APIs, so technically users can create their own equivalent with custom tweaks in userland.

yyx990803 commented 4 years ago

I think most of the design issues are settled in this RFC. I will merge this tomorrow if no major objections are raised before then.