Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Added lazy loading to jupyterlab-plotly #2260

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

Closed
pragyagarg642 wants to merge 7 commits into plotly:master from pragyagarg642:lazyload_plotly

Conversation

@pragyagarg642
Copy link

@pragyagarg642 pragyagarg642 commented Mar 10, 2020

With this change, plotly.js will be loaded on demand. This uses dynamic
imports to achieve this.

Fixes #1913

With this change, plotly.js will be loaded on demand. This uses dynamic
imports to achieve this.
Fixes plotly#1913 
Copy link
Author

@nicolaskruchten Can you please review this?

Copy link
Contributor

Yes, I will take a look. A couple of questions/concerns though: right now in JupyterLab we need to install both jupyterlab-plotly and plotlywidget and both need plotly.js loaded. Right now they both statically load the same bundle and webpack deduplicates it. Is there any way that your change could include plotlywidget as well? If not, I fear the bundle size will remain the same and we will lazy-load plotly.js.

It might be that we need to first merge the two extensions together and then make this change, but I'm not sure.

Copy link
Author

@nicolaskruchten I'm working on the changes.

Copy link
Author

@nicolaskruchten I've made changes according to your comment. Can you please review this?

Copy link
Contributor

Thanks for these changes! We're quite busy preparing for the 4.6.0 release of Plotly.py but I will review this during the 4.7.x cycle.

pragyagarg642 reacted with thumbs up emoji

Copy link
Author

Hi @nicolaskruchten , as the 4.6.0 release is complete, can you please review this?

@nicolaskruchten nicolaskruchten added this to the 4.7.0 milestone Apr 27, 2020
Copy link
Contributor

OK, I've had a chance to take a look at this PR, thanks again for submitting it! The TypeScript doesn't actually compile for me with this PR as-is, but with a few adjustments it does, and sure enough, the bundle is nicely split, loads when needed, deduplicates between the two extensions etc. I'll need to fork this PR to add my changes such that things compile, and then the rest of my team can do some QA on Windows and Linux etc, but just wanted to let you know that unless anything weird crops up, we'll be able to merge this for 4.7.

cc @emmanuelle @jonmmease

Copy link
Contributor

Update: we need to release 4.7 on Monday to meet some internal deadlines, so I'm bumping this to 4.8, which is only about 10 days away!

pragyagarg642 reacted with thumbs up emoji

This was referenced May 6, 2020
Copy link
Author

pragyagarg642 commented May 13, 2020
edited
Loading

Hi @nicolaskruchten , prefetch option can also be added to this change. With that option, module will be fetched just after the initial page load is completed and when required, module will be loaded from prefetch cache, making it faster. Without this option, it will take time to load. I can update the PR with this change as well.

Copy link
Contributor

@pragyagarg642 sure, let's see what prefetching looks like! BTW I had to make these changes #2454 to get your branch to work, if you want to incorporate them... Otherwise TypeScript compilation failed.

I should add that in order to merge this, we'll have to make equivalent changes to https://github.com/plotly/jupyterlab-chart-editor as well, as people frequently install that one too, and we want to keep all the bundle-loading logic the same. I might move that code into this repo in the very near future to keep things in sync.

I'm sorry it's taking so long to get this merged in!

Copy link
Author

@nicolaskruchten I updated the branch with required prefetch changes.

Copy link
Author

Hi @nicolaskruchten, is there any update on this or is there anything pending on my end?

Copy link
Contributor

See my comment in #2454 ... the bottleneck here is just my time in being able to QA this to the point where we're willing to roll this change out to all of our users. I'm sorry I haven't been able to find the time but this is potentially quite a risky change.

Copy link
Contributor

I do apologize about how this PR was left unattended for so long, but it was superseded by #3142 which was just merged, so the functionality will make it into the next version of the library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

upcoming

Development

Successfully merging this pull request may close these issues.

Lazy load strategy for plotly bundle

AltStyle によって変換されたページ (->オリジナル) /