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

Add async loading to reduce package size #29

Merged
merged 28 commits into from
Apr 15, 2021
Merged

Add async loading to reduce package size #29

merged 28 commits into from
Apr 15, 2021

Conversation

xhluca
Copy link

@xhluca xhluca commented Feb 19, 2021

closes #28

More information in PR #40

Screenshot of network when dash_vtk is imported but not used:
image

Pasting some explanation of the asyncComponentBuilder function that I shared with @Marc-Andre-Rivet

const AsyncComponentBuilder = name => async () => {
    const AsyncReactVTK = import(/*webpackChunkName: "ReactVTK" */ './ReactVTK');
    const ReactVTK = await AsyncReactVTK;
    // console.log("ReactVTK", ReactVTK);
    // window.ReactVTK = ReactVTK;
    const reactComponent = ReactVTK.default[name];
    // We need to trick React.lazy into thinking we are giving
    // the output of an import("my-module.js") Promise.
    const fakeModule = {default: reactComponent};
    return fakeModule;
}

Let's look at individual lines:

const AsyncReactVTK = import(/*webpackChunkName: "ReactVTK" */ './ReactVTK');

above i'm using the asynchronously importing the "react-vtk-js" library (you can ignore ./ReactVTK, it just imports * then exports the react-vtk-js library).

 const ReactVTK = await AsyncReactVTK;

above, Just resolving the promise here, so ReactVTK is an actual module instead of a promise.

const reactComponent = ReactVTK.default[name];

above i'm accessing the react component by the name, so if name="Algorithm" then it'd be equivalent to ReactVTK.default.Algorithm. Usually it would simply be ReactVTK.Algorithm but we need the default due to how i export react-vtk-js in ./ReactVTK.js .

const fakeModule = {default: reactComponent};

Just as the comment says: React.lazy expects a module, we give it an object that behaves like a module.

const AsyncComponentBuilder = name => async () => {...}

above, AsyncComponentBuilder is just a HOF that given a name (e.g. "View" or "Algorithm") will return an async function that we can give to React.lazy and use inside React.Suspense.

xhlulu added 3 commits February 18, 2021 18:37
Enables the use of the import function inside react for React.lazy
xhlulu added 3 commits February 18, 2021 20:05
This exports lazy components which are imported
in the respective component scripts. This is needed
in order for dash to detect the component and add it
to _imports_.py
@xhluca
Copy link
Author

xhluca commented Feb 19, 2021

@alexcjohnson @Marc-Andre-Rivet I pushed some commits here using Suspense and lazy. Let me know if that's what I'm supposed to do; if there's no problem I'll do the same thing for the other components.

@xhluca xhluca marked this pull request as ready for review February 19, 2021 17:45
@xhluca xhluca changed the title Add async loading for reduce weights Add async loading to reduce package size Feb 22, 2021
@xhluca
Copy link
Author

xhluca commented Apr 8, 2021

@Marc-Andre-Rivet Thanks for the advice. I had a pair-coding session with Joe and I was succefully in wrapping one of the components (dash_vtk.View) using the chunked async approach; unfortunately I started running into problems when I try to add async to the other 19 components; more about that in the slack channel #dash-vtk.

@xhluca
Copy link
Author

xhluca commented Apr 8, 2021

I'm rerolling back to before adding async so I can explore the single-chunk async apporach (load all of vtk or none).

I'm saving the state of this branch here: https://github.com/plotly/dash-vtk/tree/lazy-loading-archive-1

@xhluca xhluca mentioned this pull request Apr 12, 2021
2 tasks
* tried stuff

* ok let's go back to where it was before

* tried some more stuff that seems to make it work?

* set async to true

* Add usage simple, simplify import structure

* Generalized the async component with a async component builder function

Temporarily commenting out the other components that were causing problem

* Expand async to more components; usage.py now runs but still white square

* Improve naming for clarity

* Replace all with async react vtk

* Add more components to async-react-vtk, make builder more verbose

* Wrap Mesh with lazy/suspense

* npm run build

* Fix incorrect source map

* Add tests for docs tutorials

* Move the async import call inside the builder function

* npm run build

* Tests: Increase sleep time for demo, decrease for tutorials

* fix typo in usage-vtk-cfd

* update react-vtk-js to 1.4.1

* update generated files

* trigger circleci

* update react-vtk-js to 1.4.2

* update generated files

* trigger ci

* Apply black to all tests and usage files

* Remove dummy app

* Update code to be eslint compliant

* run build

Co-authored-by: Sebastien Jourdain <sebastien.jourdain@kitware.com>
@xhluca
Copy link
Author

xhluca commented Apr 13, 2021

@alexcjohnson @Marc-Andre-Rivet @jdamiba This PR is ready to be reviewed! I think the async should work now, and all the problem on the vtk side have been resolved by @jourdain . Feel free to take a look at the percy snapshot to validate the result.

Copy link
Contributor

@jdamiba jdamiba left a comment

Choose a reason for hiding this comment

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

This looks good to me as well. 💃🏿

I tested this locally by running the usage.py example both with and without dash-vtk components in the app layout, and can confirm that the async ReactVTK.js bundle is only served when dash-vtk components are in the layout. When the library is simply imported into an app, only the thin shell is loaded.

The same holds true at https://dash-docs-pr-1091.herokuapp.com/ and https://dash-docs-pr-1091.herokuapp.com/vtk, which are a version of the dash-docs which point to this branch of the dash-vtk library.

webpack.config.js Outdated Show resolved Hide resolved
webpack.config.js Outdated Show resolved Hide resolved
Copy link

@Marc-Andre-Rivet Marc-Andre-Rivet left a comment

Choose a reason for hiding this comment

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

💃 - @xhlulu, @jdamiba good job digging through all the details to make this (yet to be documented feature) work for this package 👍

@xhluca
Copy link
Author

xhluca commented Apr 15, 2021

Thank you @Marc-Andre-Rivet @jdamiba @jourdain! Merging this now.

@xhluca xhluca merged commit efc6a09 into master Apr 15, 2021
@xhluca xhluca deleted the lazy-loading branch April 15, 2021 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Async loading of dash-vtk for multi-page apps
4 participants