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 inline argument to chart.save() for html export #2807

Merged
merged 6 commits into from
Jan 8, 2023

Conversation

jonmmease
Copy link
Contributor

This includes the Vega/Vega-Lite/Vega-Embed JavaScript source in the exported html file so that it works offline. The logic is ported from altair_saver.

See #2765

Note: Like altair_saver, this pulls the JavaScript source from the altair_viewer package. In the future we could talk about whether to include the JavaScript bundles in Altair itself to avoid this dependency.

This includes the Vega/Vega-Lite/Vega-Embed JavaScript source in the exported html file so that it works offline.  The logic is ported from altair_saver.
@jonmmease
Copy link
Contributor Author

Seeing a couple of CI errors with altair_saver and Python 3.11. Does this look familiar to anyone? Should be try kicking off the tests again?

Copy link
Contributor

@joelostblom joelostblom left a comment

Choose a reason for hiding this comment

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

I reran the test and it is passing now. I think this looks good overall, just one or two questions in the comments. @mattijn does this approach makes sense to you too?

Comment on lines +255 to +261
if template == "inline":
try:
from altair_viewer import get_bundled_script
except ImportError:
raise ImportError(
"The altair_viewer package is required to convert to HTML with inline=True"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this approach versus adding altair_viewer as a required dependency for altair

altair/utils/save.py Show resolved Hide resolved
altair/utils/save.py Outdated Show resolved Hide resolved
@jonmmease
Copy link
Contributor Author

Also, I tested this manually. But it would be good for someone else to try it out manually as well since the test case for checking that it's working just checks for an expected substring that's different that what's expected with inline=False.

Co-authored-by: Mattijn van Hoek <mattijn@gmail.com>
@mattijn
Copy link
Contributor

mattijn commented Jan 7, 2023

@jonmmease: out of curiosity: where does vl-convert get the javascript bundles of vega and vega-lite from?

@jonmmease
Copy link
Contributor Author

where does vl-convert get the javascript bundles of vega and vega-lite from?

It pulls them from the Skypack CDN, which integrates well with Deno. The kind of interesting thing is that there's no global bundler, VlConvert inlines a bunch of individual bundles (for each dependency) and Deno looks them up dynamically on import.

This is the directory of source files that are inlined into into VlConvert on build: https://github.com/vega/vl-convert/tree/main/vl-convert-rs/vendor.

I was thinking about whether there would be a good way for vl-convert to inline these into a standalone HTML file, but I'm not sure how to manually bundle them so that imports work.

I was thinking about whether there would be a good way to use these

@mattijn
Copy link
Contributor

mattijn commented Jan 7, 2023

or how altair could use these if vl-convert-python is installed..
Maybe related: #2816

@mattijn
Copy link
Contributor

mattijn commented Jan 7, 2023

Regarding this PR, I can understand the intention, but I trust you here @joelostblom. I never have used this functionality in altair_saver. If the tests don’t cover what should be tested, maybe we can add some tests that do cover what is needed to check this?
I had one question, after this PR, can you still use the same functionality from altair_saver?

Copy link
Contributor

@joelostblom joelostblom left a comment

Choose a reason for hiding this comment

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

I tested manually (using the 4.17.0 vega-lite version in altair viewer) and this works as expected.

I had one question, after this PR, can you still use the same functionality from altair_saver?

I tested both with and without altair_saver installed and the changes in this PR takes precedence so if you are using a version of altair from after this PR was merged, then you will not use the altair_saver functionality. However, I think it makes sense to keep that functionality around in altair saver to support older version of altair.

Merging, thanks @jonmmease !

@joelostblom joelostblom merged commit 291bcfc into vega:master Jan 8, 2023
@mattijn
Copy link
Contributor

mattijn commented Jan 8, 2023

Note: once Altair v5 is released, this feature will only work correctly after a new release of altair_viewer as well. Since the current release of altair_viewer (v0.4.0) does not include the JavaScript source of vega-lite v5, but just v4 (v4.17.0).
The main branch of the repository on Github of altair_viewer already contains VL5 javascript sources (currently v5.2).

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.

3 participants