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

Bundle external JS dependencies for custom models #1651

Merged
merged 17 commits into from
Oct 19, 2020
Merged

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Oct 19, 2020

During build time this PR would pull all external resources specified on Model.__javascript__ definitions and bundle them into the dist folder, which means when resources='server' they could be served locally.

The downside to this approach is that this grows the Python distribution by about 9 MB and this is only set to grow as more models are added. However this is the only approach that will allow these models to work in an air-gapped environment.

  • Switch __javascript__ dynamically depending on the current resources setting

@mattpap @MarcSkovMadsen @jbednar @xavArtley Opinions welcome here

@jlstevens
Copy link
Contributor

jlstevens commented Oct 19, 2020

I think at this stage of the project it is a good idea. That said, I think we should decide on a size limit and put it in the tests...e.g a 15MB limit for now and fail if we grow past that. Whether automated or not, I think we need to keep an eye on the total size to make sure it doesn't explode.

Of course, when it makes sense things should be split up into separate packages/extensions...

@codecov
Copy link

codecov bot commented Oct 19, 2020

Codecov Report

Merging #1651 into master will decrease coverage by 0.30%.
The diff coverage is 62.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1651      +/-   ##
==========================================
- Coverage   85.41%   85.10%   -0.31%     
==========================================
  Files         148      148              
  Lines       16840    16944     +104     
==========================================
+ Hits        14384    14421      +37     
- Misses       2456     2523      +67     
Impacted Files Coverage Δ
panel/tests/test_server.py 100.00% <ø> (ø)
setup.py 0.00% <0.00%> (ø)
panel/io/notebook.py 57.98% <12.50%> (-1.16%) ⬇️
panel/compiler.py 11.53% <16.39%> (+6.88%) ⬆️
panel/io/resources.py 79.72% <81.48%> (-12.34%) ⬇️
panel/models/ace.py 95.23% <83.33%> (-4.77%) ⬇️
panel/models/echarts.py 92.30% <83.33%> (-7.70%) ⬇️
panel/models/plotly.py 95.83% <83.33%> (-4.17%) ⬇️
panel/models/vega.py 91.66% <83.33%> (-8.34%) ⬇️
panel/models/vtk.py 98.38% <83.33%> (-1.62%) ⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1088ce1...be6967a. Read the comment docs.

@philippjfr
Copy link
Member Author

So this will not work for all external dependencies, in particular if a dependency expects other files to be present alongside the main js/css files. As an example katex expects various woff font files to be present. So for now I'd only enable this for a subset of models.

@philippjfr philippjfr merged commit 3961571 into master Oct 19, 2020
@philippjfr philippjfr deleted the bundle_resources branch October 19, 2020 19:45
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.

None yet

2 participants