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

Use dynamic import for mxgraph to reduce final main bundle size #19

Closed
wants to merge 13 commits into from

Conversation

bollwyvl
Copy link
Contributor

@bollwyvl bollwyvl commented Jun 5, 2018

Thanks for the work!

@SylvainCorlay and I talked about some ways to make this extension more adoptable, and a big one is getting the end-user build burden under control... all it takes is a few big dependencies, and end users will start running into out of memory errors for production builds.

This PR uses a dynamic import().then((mx)=>...) for mxgraph which moves the vendored code into its own separate bundle, enabled by a tweak to tsconfig.json and some changes to editor.ts.

While it still imports ./mxgraph/.../modulated.js for typing, this doesn't trigger bundling into main, and a new promise handles some housekeeping until the extra bundle loads. The net effect is a lot less spinning moons up-front and a lot more spinner the first time you create a drawio editor, but this seems like a reasonable trade.

Performance

jupyter lab build before dynamic loading:

Time: 48291ms
                                 Asset       Size  Chunks                    Chunk Names
...
             0.279ec850a9242db04c85.js     465 kB       0  [emitted]  [big]  
          main.61b90d338d4f30708998.js    3.82 MB       1  [emitted]  [big]  main
        vendor.dc51f756bf07759c9f87.js    1.57 MB       2  [emitted]  [big]  vendor
      manifest.a4f4df6bc82fcc2452d4.js    1.62 kB       3  [emitted]         manifest
         0.279ec850a9242db04c85.js.map    1.35 MB       0  [emitted]         
      main.61b90d338d4f30708998.js.map    14.6 MB       1  [emitted]         main
    vendor.dc51f756bf07759c9f87.js.map    5.69 MB       2  [emitted]         vendor
  manifest.a4f4df6bc82fcc2452d4.js.map    8.24 kB       3  [emitted]         manifest

jupyter lab build after dynamic loading:

Time: 41686ms
                                 Asset       Size  Chunks                    Chunk Names
...
             0.279ec850a9242db04c85.js     465 kB       0  [emitted]  [big]  
             1.1744eaa7d53b3aae4ebe.js    1.51 MB       1  [emitted]  [big]  mxgraph
          main.395f70c2ada21ac40f18.js    2.31 MB       2  [emitted]  [big]  main
        vendor.a77cf169b5927fb4a051.js    1.57 MB       3  [emitted]  [big]  vendor
      manifest.b77db697007c7c420aa4.js    1.65 kB       4  [emitted]         manifest
         0.279ec850a9242db04c85.js.map    1.35 MB       0  [emitted]         
         1.1744eaa7d53b3aae4ebe.js.map    5.41 MB       1  [emitted]         mxgraph
      main.395f70c2ada21ac40f18.js.map    9.16 MB       2  [emitted]         main
    vendor.a77cf169b5927fb4a051.js.map    5.69 MB       3  [emitted]         vendor
  manifest.b77db697007c7c420aa4.js.map    8.29 kB       4  [emitted]         manifest

The slightly shorter build time could be incidental, but the size difference is pretty real...

Also, since i was doing a bunch of builds, I snuck in the spdx-compatible license string because i got tired of the warnings.

The other thing to making this more adoptable (out of scope for this PR) is some strategy for removing the runtime github.com dependency. Not sure the right way to do it: one could make an nbextension, and pack all that stuff in there, but that feels really dirty. I am figuring we'll end up needing a predictable, runtime-addressable spot to put statics, especially for wasm and complicated existing apps like this-a-here one.

Thanks again!

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Nov 1, 2019

Hey, folks: warmed this back up!

Here's the packed size:

notice === Tarball Details ===
npm notice name:          jupyterlab-drawio
npm notice version:       0.6.0
npm notice filename:      jupyterlab-drawio-0.6.0.tgz
npm notice package size:  3.6 MB
npm notice unpacked size: 15.3 MB
npm notice shasum:        9f8acb5be507991b02e07152eed77bcc73747ca1
npm notice integrity:     sha512-jyYY928bxQaSB[...]mHQZaNQJDq8Sw==
npm notice total files:   272
npm notice
jupyterlab-drawio-0.6.0.tgz

Here's the final bundles (under a prod, minimized build):

      vendors~main.0602e72ecb5c423a90d2.js (4.23 MiB)
      vendors~mxgraph.11d4c5c0f9b41a26b41e.js (1.44 MiB)

I can't do anything about main having gotten bigger (i tried, i really tried, but blueprint + everything...) but mx is still looking good: certainly reasonable for what it delivers.

For giggles, and in lieu of automated testing, I hoisted binder to the repo root so that it can be evaluated interactively: Binder. On binder, there is a noticeable delay before loading the first drawing, but the it's undetectable. It also cleans up fairly agressively after itself, such that subsequent image launches are snappier.

If we can get some of this size management stuff in place, I'd love to see this move forward, and potentially investigate bumping to the newer revs (a la #43), or even taking a crack at mxgraph2. I'd also take a stab at spinning up some way to get off the github runtime dependency (sneaky nbextension? serverextension? configurable static url setting with fallback?) Once it was a bit more predictable in how it builds, and how it might offer extension points, there could be a lot of really good stuff... over on jupyterlab-lsp we've been mulling about how to pull over some features from lintotype... it would be brilliant to be able to generate diagrams of code, and use them to navigate your files.

@wolfv
Copy link
Member

wolfv commented Nov 4, 2019

I like it! I am happy to merge ithis :) I might even steal your trick for some other extensions;

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Nov 4, 2019

Cool!

On terminal, we went a slightly different way... i'll do an alternate PR with that approach.

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Nov 4, 2019

Uhoh, ABCWidgetFactory.createNewWidget can't be async, so it would take a fair amount of doing: you'd need a shim to make the loading more seamless, so though I'll take another look, I guess this as well as we can do right now (in that it works).

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.

2 participants