wix-incubator / react-module-container

Small library for building micro apps in React and Angular
MIT License
69 stars 18 forks source link

feat(lazy load): add manifest support for resolve #9

Closed tomerla closed 7 years ago

tomerla commented 7 years ago

This is an early-stage PR for adding support for resolve property in the manifest

Motivation: while the browsing is (lazy) downloading the manifest files, we can simultaneously do ajax request to fetch custom data before rendering the widget

odedcagan commented 7 years ago

In React the hosted module can do this on its own:

window.ModuleRegistry.registerComponent('appName.mainComponentName', () => withData(MainComponent));
function withData(WrappedComponent) {
  class PrepareApp extends Component {
    constructor(props) {
      super(props);
      this.state = {data: null};
    }

    componentDidMount() {
      fetchData().then(data => {
        this.setState({data});
      });
    }

    render() {
      return this.state.data ?
      React.createElement(WrappedComponent, {data}) :
      null;
    }
  }
}
tomerla commented 7 years ago

@odedcagan yes, it would definitely do the trick although I still prefer the resolve pattern since we can fetchData() in parallel with the browser downloading the bundles

odedcagan commented 7 years ago

@tomerla oh I missed that, fetching data in parallel sounds very interesting. A few things though:

odedcagan commented 7 years ago

@shahata wdyt?

shahata commented 7 years ago

@odedcagan @tomerla I like the idea, but plz make sure api makes sense and code is reused correctly between angular and react lazy loaders. I'd like to review when you two are happy and ready to merge.

tomerla commented 7 years ago

@odedcagan PR is ready, please review

I decided to use your suggestion (so resolve will be a function of type TResolve = () => Promise<object> instead of an object)

Please notice that there 2 commits for this PR

  1. Adding resolve to the manifest
  2. Code reuse for the resolve logic inside a BaseLazyComponent

I think it would be easier to review them separately...

odedcagan commented 7 years ago

@tomerla Thank you and good job! 👏

@shahata almost ready to merge, could you review as well?

tomerla commented 7 years ago

@odedcagan I updated the docs with the types of resolve and prepare

Regarding the Object.assign - It requires a polyfill so I thought its better to avoid using it

odedcagan commented 7 years ago

@tomerla looks good, @shahata ?

shahata commented 7 years ago

Very nice work @tomerla! Merging.