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

Support for Dynamic Imports #13

Closed
hutchgrant opened this issue Feb 11, 2019 · 7 comments
Closed

Support for Dynamic Imports #13

hutchgrant opened this issue Feb 11, 2019 · 7 comments
Labels
feature New feature or request

Comments

@hutchgrant
Copy link
Contributor

hutchgrant commented Feb 11, 2019

Is your feature request related to a problem? Please describe.
First, I've been following this project for a few months now and found it to be exactly what I need for my lit-element web component needs. There's one thing I think it's missing, and that's route based lazy loading of components.

Describe the solution you'd like
On route change, it should load an optional developer provided loading component. It should make an asynchronous request for the component associated with the route, then it should replace the loading component with the requested component once the promise is resolved. The additions can be added to the route.ts.

Describe alternatives you've considered
I considered and demonstrated the use of a higher order component to achieve the same result using lit-redux-router. Although, it isn't the cleanest way of doing it. The better way is to modify lit-redux-router. I thought it might be helpful to contribute it to you first. Maybe you have a better idea for how to approach this.

Additional context

An example of what two lazy loaded component routes would look like:

import { html, LitElement } from 'lit-element';
import { connectRouter } from 'lit-redux-router';
import store from './store.js';

import '../pages/home/home.js';
import '../components/Loading';

connectRouter(store);

class AppComponent extends LitElement {

  render() {
    const AboutRoute = {
      load: () => import(/* webpackChunkName: "aboutPage" */ '../pages/home/about'),
      loading: 'eve-loading'
    };
    const DocsRoute = {
      load: () => import(/* webpackChunkName: "docsPage" */ '../pages/home/docs'),
      loading: 'eve-loading'
    };

    return html`
      <div>
        <lit-route path="/" component="home-page"></lit-route>
        <lit-route path="/about" component="about-page" lazy="${true}" .loader="${AboutRoute}"></lit-route>
        <lit-route path="/docs" component="docs-page" lazy="${true}" .loader="${DocsRoute}"></lit-route>
      </div>
    `;
  }

}

customElements.define('eve-app', AppComponent);
@thescientist13
Copy link

thescientist13 commented Feb 11, 2019

@hutchgrant
Curious, does this assume a hard dependency on webpack?

const AboutRoute = {
  load: () => import(/* webpackChunkName: "aboutPage" */ '../pages/home/about'),
  loading: 'eve-loading'
};
const DocsRoute = {
  load: () => import(/* webpackChunkName: "docsPage" */ '../pages/home/docs'),
  loading: 'eve-loading'
};

@thescientist13
Copy link

thescientist13 commented Feb 11, 2019

oh I see, this is just for your example usage, and not needed for the underlying implementation?

@hutchgrant
Copy link
Contributor Author

Yea that's just the example, you don't have to use webpack chunks. It does however assume the babel dynamic import plugin will be used in the implementation. But that's optional, for this optional feature. It's not a dependency for the module.

@fernandopasik fernandopasik added the feature New feature or request label Feb 13, 2019
@fernandopasik
Copy link
Owner

Thanks for opening this issue @hutchgrant, it will be a great addition!

@fernandopasik
Copy link
Owner

fernandopasik commented Feb 13, 2019

This problem intersects with another use case that is if you have to query for data or any other async process before being able to display a route. (angular ui-router had an interface for this for example)

calling an api using a promise

<lit-route 
    path="/docs" 
    component="docs-page" 
    .resolve="${() => fetch('mydomain.com/api')}"
></lit-route>

dynamic import is a promise as well

<lit-route 
    path="/docs" 
    component="docs-page" 
    .resolve="${() => import(/* webpackChunkName: "aboutPage" */ '../pages/home/about')}"
></lit-route>

As you suggested we need a component for loading status

<lit-route 
    path="/docs" 
    component="docs-page" 
    .resolve="${() => import(/* webpackChunkName: "aboutPage" */ '../pages/home/about')}"
    loading="my-loading"
></lit-route>

And potentially add onSuccess and onError events for redirects or tracking or whatever

<lit-route 
    path="/docs" 
    component="docs-page" 
    .resolve="${() => import(/* webpackChunkName: "aboutPage" */ '../pages/home/about')}"
    loading="my-loading"
    @success="${result => console.log(result)}"
    @error="${error => console.log(error)}"
></lit-route>

@fernandopasik
Copy link
Owner

fernandopasik commented Feb 14, 2019

I suggest the following implementation with a few changes in the stateChanged lifecycle and the render function

public stateChanged(state: State): void {
  this.active = isRouteActive(state, this.path);
  this.params = getRouteParams(state, this.path);

  if (this.active && this.resolve) {
    this.isResolving = true;
    this.resolve()
      .then(() => {
        this.isResolving = false;
        // here potentially fire custom success event
      })
      .catch(() => {
        this.isResolving = false;
        // here potentially fire custom error event
      });
  }
}
public render(): TemplateResult {
  if (!this.active) {
    return html``;
  }

  if (this.resolve && this.isResolving) {
    return !this.loading ? html`` : this.getTemplate(this.loading);
  }

  if (!this.component) {
    return html`<slot></slot>`;
  }

  return this.getTemplate(this.component, this.params);
}

@hutchgrant
Copy link
Contributor Author

hutchgrant commented Feb 14, 2019

That looks good to me, though I still have to test it. That's way better looking logic than my hacky method.

Also the success and error events should be optional as well.

fernandopasik pushed a commit that referenced this issue Feb 15, 2019
* Adding Lazy Loading, resolves #13

* fix: lint issues

* fix: removing package-lock.json

* fix: fixing demo

* docs: adding dyn route

* fix: resolve property init and removed comments

* fix: remove demo route

* test: dyn component and loading

* test: more dyn tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants