kaisermann / svelte-loadable

Dynamically load a svelte component
MIT License
322 stars 13 forks source link

Loader delay #1

Closed EmilTholin closed 6 years ago

EmilTholin commented 6 years ago

This component is super promising!

What do you think about stealing the idea of a loader delay from react-loadable? This makes it so you don't get a flashing loading indicator for components that are very fast to load. I'm not quite sure how the API for that would look, but maybe something along these lines:

<!-- App.html -->
<Loadable ref:loadable {loader} {delay} bind:error bind:pastDelay>
  {#if pastDelay}
    <div slot="loading">Loading but doomed to failed...</div>
  {/if}
  <div slot="error">
    {error}
    <br>
    <button on:click="refs.loadable.load()">Try again</button>
  </div>
</Loadable>

<script>
  export default {
    components: {
      Loadable: '../../Loadable.html'
    },
    data() {
      return {
        loader: () => import('./AsyncComponent.html'),
        delay: 300
      }
    }
  }
</script>

And the load function could maybe be augmented with something like this:

const { loader, delay } = this.get()
if (typeof loader === 'function') {
  this.set({
    state: STATES.LOADING,
    error: null,
    component: null,
  })

  if (delay !== 0) {
    setTimeout(() => {
      const { state } = this.get();

      if (state === STATES.LOADING) {
        this.set({ pastDelay: true });
      }
    }, delay);
  }

  loader()
    .then(componentModule => {
      this.set({
        state: STATES.SUCCESS,
        component: componentModule.default || componentModule,
      })
    })
    .catch(e => {
      this.set({
        state: STATES.ERROR,
        error: e,
      })
    })
}
kaisermann commented 6 years ago

That's a nice addition! What about wrapping the whole load call inside the setTimeout and default the delay to 0?

      load() {
        const { loader, delay } = this.get()
        if (typeof loader === 'function') {
            this.set({
              state: STATES.LOADING,
              error: null,
              component: null,
            })

          setTimeout(() => {
            loader()
              .then(componentModule => {
                this.set({
                  state: STATES.SUCCESS,
                  component: componentModule.default || componentModule,
                })
              })
              .catch(e => {
                this.set({
                  state: STATES.ERROR,
                  error: e,
                })
              })
          }, parseFloat(delay))
        }
      },

(oh, and we can't fill a slot inside a {#if} 😱)

kaisermann commented 6 years ago

This component is super promising!

🀣

EmilTholin commented 6 years ago

I think wrapping the whole load call inside the setTimeout would delay the actual fetch of the component from the network, which would not be ideal. Ideally the import starts right away, but the loading indicator just has a delay, so that the loading indicator will not be shown if it takes less than delay ms to import the component.

(oh, and we can't fill a slot inside a {#if} 😱)

Ah, shoot. Didn't think about that.

kaisermann commented 6 years ago

Ahh! I understood something completely different. I thought you wanted to have a "minimum" load time. Let me see what I come up with πŸ€”

kaisermann commented 6 years ago

What about cancelling the loading state setTimeout after succeeding or failing?

      load() {
        const { loader, minLoadTime } = this.get()
        if (typeof loader === 'function') {
          const loadTimeout = setTimeout(() => {
            this.set({
              state: STATES.LOADING,
              error: null,
              component: null,
            })
          }, parseFloat(minLoadTime))

          loader()
            .then(componentModule => {
              this.set({
                state: STATES.SUCCESS,
                component: componentModule.default || componentModule,
              })
              clearTimeout(loadTimeout)
            })
            .catch(e => {
              this.set({
                state: STATES.ERROR,
                error: e,
              })
              clearTimeout(loadTimeout)
            })
        }
      },
EmilTholin commented 6 years ago

That code looks really clean! I personally think minLoadTime is a bit misleading, since it's not delaying rendering of the imported component, but delaying the rendering of the loading indicator, but now I'm just nitpicking.

kaisermann commented 6 years ago

As usual, we're dealing with the most difficult thing that a developer can deal with:

naming things

Any suggestions πŸ˜€ ?

EmilTholin commented 6 years ago

I'm a horrible person; criticizing a name, but not having a suitable substitute.

I like delay from react-loadable, but maybe that's too generic?

kaisermann commented 6 years ago

Already commited it! Went with delay πŸ˜€