kaisermann / svelte-loadable

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

feat: add ability to unload component and optionally callback #23

Closed clintwood closed 4 years ago

clintwood commented 4 years ago

This PR adds the ability to unload (or prevent caching) of a component with the additional option of a callback being called when the component is removed from the cache.

As described in the updated readme, an additional prop unload was added which is of type boolean or () => void. If true (default false) the component (and loader) will be removed from the LOADED cache/Map. If unload is a callback it is called immediately after the component is removed from the cache.

Rationale: For components which are rarely used (say settings) or a complex web app where there are a large number of dynamic components (which may utilise considerable memory) it is nice to know the components are not stacking up in a cache.

kaisermann commented 4 years ago

I can certainly see the rationale behind having a way to unload some components from a large application. However, the name unload doesn't strike me directly as a way to unload from the cache once the component is unmounted, at least for me. Maybe a unloadOnDestroy would be a better fit, albeit more verbose 🤔 What do you think (i'm really not sure ahaha)?

About the callback variant, what kind of use cases do you imagine for it? While typing this comment, I also thought about instead of passing a callback to execute it on the onMount return, we could dispatch a destroy event (a la svelte v2):

  <Loadable loader={...} unload={true} on:destroy={() => console.log('bye bye')} />
  <!-- or -->
  <Loadable loader={...} unload={true} on:unmount={() => console.log('bye bye')} />

By doing this users would be able to pass any callback, even if they don't want to unload their component.

clintwood commented 4 years ago

There is a more symmetrical way to do this which would be to rather just add an unloader prop which expects a function. If a unloader is passed then it is called and the cached components is released (deleted). The only downside is you'd have to pass a () => {} (noop) if all you wanted to do was remove the component from the cache.

I'm also more in favour of using the callback as opposed to the event since for my case I'd prefer to execute the callback as Loadable is being destroyed and not have the overhead of an event floating around in Loadable when this feature would probably very rarely be used. This is purely my opinionated preference - this is your call :grin:

My specific use case is that I'm developing a hybrid mobile app that can load a number of potentially large bundled modules using SystemJS which has the ability to also delete (unload) previously loaded modules thus decreasing the memory footprint of the app. This PR's feature would also be very useful for a plugin type application where a plugin could be loaded and unloaded.

In the meantime I will rename the prop to unloadOnDestroy and I'll wait on any other changes you want. Also feel free to change it completely - I have no issue with that!

clintwood commented 4 years ago

Having considered what we've been discussing I think the cleanest option is to to go symmetrical with the prop name unloader which is either a boolean or a function (no text 'true'/'false' as you rightly pointed out). In both cases if set (i.e. either true or a function) the component will be released from the cache. If unloader is a function it will be called immediately after the component is released from the cache.

I think this keeps the Loadable interface clean and its fairly common to have a single prop supporting multiple types if it is well documented!

Examples below:

<script>
  function unloader() {
    /* some code here */
  }
</script>

<!-- unloader as boolean -->
<Loadable ... unloader />
<Loadable ... unloader={true} />
<Loadable ... unloader={someBooleanValue} />

<!-- unloader as a function -->
<Loadable ... {unloader} />
<Loadable ... unloader={() => { /* some code here */ }} />
kaisermann commented 4 years ago

@clintwood That I like! Seems good to me 😁

clintwood commented 4 years ago

Awesome - will update!

clintwood commented 4 years ago

Done!

kaisermann commented 4 years ago

Just some silly changes and we're good to go!

kaisermann commented 4 years ago

Released in v1.4.0 🎉