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

New Async Component API #148

Merged
merged 5 commits into from
Apr 9, 2020
Merged

New Async Component API #148

merged 5 commits into from
Apr 9, 2020

Conversation

yyx990803
Copy link
Member

@yyx990803 yyx990803 commented Mar 25, 2020

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

@yyx990803 yyx990803 added 3.x This RFC only targets 3.0 and above breaking change This RFC contains breaking changes or deprecations of old API. core labels Mar 25, 2020
@cexbrayat
Copy link
Member

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
Copy link
Contributor

CyberAP commented Mar 26, 2020

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
Copy link
Contributor

CyberAP commented Mar 26, 2020

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
Copy link
Contributor

CyberAP commented Mar 26, 2020

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

@yyx990803
Copy link
Member Author

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

@cexbrayat
Copy link
Member

@yyx990803 Sure, here it is vuejs/core#888 Thank you for considering it.

@yyx990803
Copy link
Member Author

yyx990803 commented Mar 26, 2020

@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
Copy link
Contributor

CyberAP commented Mar 26, 2020

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
Copy link
Member Author

yyx990803 commented Mar 26, 2020

@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
Copy link

leopiccionia commented Mar 26, 2020

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
Copy link
Member Author

yyx990803 commented Mar 26, 2020

@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
})

Copy link

@jacekkarczmarczyk jacekkarczmarczyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR description should be updated too
image

@yyx990803
Copy link
Member Author

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.

@yyx990803 yyx990803 added the final comments This RFC is in final comments period label Mar 31, 2020
@Morgul
Copy link

Morgul commented Apr 6, 2020

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
Copy link
Contributor

CyberAP commented Apr 6, 2020

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
Copy link
Member Author

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
Copy link

backbone87 commented Apr 7, 2020

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
Copy link
Contributor

CyberAP commented Apr 7, 2020

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
Copy link
Member Author

yyx990803 commented Apr 7, 2020

@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.

yyx990803 added a commit to vuejs/core that referenced this pull request Apr 7, 2020
BREAKING CHANGE: `retryWhen` and `maxRetries` options for
`defineAsyncComponent` has been replaced by the more flexible `onError`
option, per vuejs/rfcs#148
Comment on lines 114 to 126
``` js
const Foo = defineAsyncComponent({
// ...
onError(error, retry, fail, attempts) {
if (error.message.match(/fetch/) && attempts <= 3) {
// retry on fetch errors, 3 max attempts
retry()
} else {
fail()
}
}
})
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 am ok with that idea as well. Could onError arguments be contained in a single destructurable object? So there's no need to remember the exact order in which they're passed to a callback.

Suggested change
``` js
const Foo = defineAsyncComponent({
// ...
onError(error, retry, fail, attempts) {
if (error.message.match(/fetch/) && attempts <= 3) {
// retry on fetch errors, 3 max attempts
retry()
} else {
fail()
}
}
})
```
``` js
const Foo = defineAsyncComponent({
// ...
onError({ error, retry, fail, attempts }) {
if (error.message.match(/fetch/) && attempts <= 3) {
// retry on fetch errors, 3 max attempts
retry()
} else {
fail()
}
}
})

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, the order feels pretty natural to me. With destructure instead of the order, you'll have to remember the exact names. It's not much different imo.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its less about remembering imho, but more about needing only a few of the args most of the time, depending on the use case. See my examples

@Morgul
Copy link

Morgul commented Apr 7, 2020

@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
Copy link
Member Author

@Morgul error has been renamed to errorComponent.

@backbone87
Copy link

backbone87 commented Apr 8, 2020

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
Copy link
Member Author

@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
Copy link

backbone87 commented Apr 8, 2020

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

  • provide a defineAsyncComponent which solves the 80% use-cases (timeout, retry, retryWhen, loadingComponent, delay, errorComponent)
  • expose the "atoms" of how defineAsyncComponent is assembled to support custom use-cases (this part of the API could be marked experimental in the beginning until the surface settles over the first few minor versions)

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
Copy link
Member Author

@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
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x This RFC only targets 3.0 and above breaking change This RFC contains breaking changes or deprecations of old API. core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants