Add 'plotly_noembed' cargo feature #231
Conversation
sharkdp
force-pushed
the
add-no_embed-feature
branch
from
4fa6722 to
6fff5de
Compare
plotly/src/plot.rs
Outdated
| #[cfg(feature = "plotly_noembed")] | ||
| #[derive(Template)] | ||
| #[template(path = "static_plot_noembed.html", escape = "none")] | ||
| #[cfg(not(target_family = "wasm"))] | ||
| struct StaticPlotTemplate<'a> { | ||
| plot: &'a Plot, | ||
| format: ImageFormat, | ||
| width: usize, | ||
| height: usize, | ||
| } |
There was a problem hiding this comment.
Not sure if there is a way to do this without having to duplicate the whole struct.
|
@sharkdp , thank you for submitting the PR! |
This adds a new `plotly_noembed` feature that can be used to reduce the
sizes of binaries by not embedding `plotly.min.js`.
Since askama can not handle compile-time flags, we had to duplicate the
`plot.html` and `static_plot.html` templates, where the new
`*_noembed.html` version only contains the CDN-variant.
The `use_local_plotly` option has been feature-gated such that it is
not available when using the new feature.
I verified that this works by building an application that depends on
plotly.rs (including optimization and LTO in all cases):
no plotly: 4.96 MiB (5,201,920 bytes)
master: 8.96 MiB (9,400,320 bytes)
this branch: 5.44 MiB (5,701,632 bytes)
The difference between this branch and master is 3,698,688 bytes, which
is very close to the size of `plotly.min.js` (3,686,400 bytes). Just as
expected.
andrei-ng
force-pushed
the
add-no_embed-feature
branch
from
6fff5de to
e3052a5
Compare
|
Hi @sharkdp, I had a chance to look into the PR today. Indeed, duplicating the template files is annoying. I have looked into what alternatives we have. TBH, I thought about it and I think the embedding of the Also using the if/else in the template as it was originally implemented causes this issue of not being able to use the Rust compile time configuration as you pointed out in the #176 . We might be able to bypass that now since thanks to @GuillaumeGomez , we switched to I propose for the moment a simpler approach, where we always use CDN and we bake in directly the local version only with the opt-in flag. I am going to push a commit on top of yours with these changes. Would be interested to hear your opinion. |
|
I think that is a good idea. CDN as default sounds like a reasonable default to me, given that the local plotly version is so large. Feel free to amend this PR as you see fit. Thank you |
andrei-ng
force-pushed
the
add-no_embed-feature
branch
from
7163d7a to
4e7d681
Compare
Great! Then I will merge with the changes I made on top of yours. If you spot issues when you use the upcoming version, let me know. |
Signed-off-by: Andrei Gherghescu <8067229+andrei-ng@users.noreply.github.com>
andrei-ng
force-pushed
the
add-no_embed-feature
branch
from
4e7d681 to
1fe6f11
Compare
|
Awesome. I just tested it and it still works as expected. I'll integrate it into Numbat as soon as the release is out. Thank you 👍 |
|
I'll make a new release tomorrow. |
@sharkdp FYI, released 0.10.0 version just now. |
|
Thank you! It's now integrated into Numbat 👍 |
This adds a new
plotly_noembedfeature that can be used to reduce the sizes of binaries by not embeddingplotly.min.js.Since askama can not handle compile-time flags, we had to duplicate the
plot.htmlandstatic_plot.htmltemplates, where the new*_noembed.htmlversion only contains the CDN-variant.The
use_local_plotlyoption has been feature-gated such that it is not available when using the new feature.I verified that this works by building an application that depends on plotly.rs (including optimization and LTO in all cases):
The difference between this branch and master is 3,698,688 bytes, which is very close to the size of
plotly.min.js(3,686,400 bytes). Just as expected.closes #176