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
Ship vega4-extension by default #6591
Conversation
Thanks for making a pull request to JupyterLab! To try out this branch on binder, follow this link: |
I think I've gotten myself a bit confused. If both extensions only import |
The vega4 package depends on a different version of Vega and Vega-Lite. I was hoping that that would be sufficient. Vega-Embed does not depend on Vega or Vega-Lite directly. Instead, they are specified as peer dependencies. |
Does Jupyterlab allow multiple versions of the same package at the same time? If not, we may need an alias release |
See recent comments on #6572 - I think we are converging on this being a good idea, but it looks like there are still details to work out. I too am confused on how this will work as |
Vega-Embed now uses peer dependencies. This means that Vega and Vega-Lite have to be installed but it doesn't depend on specific versions. |
@domoritz yep that makes sense. We depend on the the version of vega and vega-lite now in the vega extension. This is currently failing some tests. I am going to work on this locally to try to get it working and test it. |
@domoritz I have made some progress here, but I am having an issue I think because vega-lite 2.6.0's dependency on vega-util is too broad https://github.com/vega/vega-lite/blob/v2.6.0/package.json#L116 It depends on
In the mentioned
which looks like it is missing an |
I opened a PR against this branch which gets things working locally for me: domoritz#1 |
In my branch, outputs of both version work: https://gist.github.com/saulshanabrook/97c28550ab12e684adc3c325038537ce |
@saulshanabrook Does it correctly delegate to the right extension? That is, does v2 go to the vega4-extension, and v3 go to the vega5-extension? Or is it just delegating to one of them that happens to be able to render both schemas? If it is delegating correctly, I'd be curious to do some debugger tracing to see how that dispatch is actually happening, since I don't understand it just yet. |
#6591 (comment) should be fixed in Vega-Lite 2.7, which I just released. |
Otherwise we don't hoist the widgets package and end up with two versions
Fixes vega lite 4 extensions
The extractor supports a function for public path, that could be made to work: https://github.com/webpack-contrib/mini-css-extract-plugin#publicpath |
@saulshanabrook Does all this custom code in buildutils mean that third-party extensions will not be able to accomplish such a side-by-side setup? |
(Trying to ensure we stick to our promise that core extensions are not "special" in any way) |
Is the idea here that the public path function relies on a base url from the page config? |
I guess what I said doesn't make sense, since the public path function is run at webpack compile time, not runtime: https://github.com/webpack-contrib/mini-css-extract-plugin/blob/392c4ae68c9384230d6652e7fee277a702deacd7/src/loader.js#L75 |
IIRC, all of the buildutils here are integrity checks on our own packages, not necessary things that all packages need. |
Ah, right. It could be that we need to use a separate endpoint for known extracted css that uses the same logic as the theme loader. That is yet another asset class that can't be put on a cdn then... |
Interestingly, looking at the generated css, the only urls seem to be inlined data urls, and things that look like this for the fonts that are beside the css file:
which is coming from setting the webpack config at jupyterlab/dev_mode/webpack.config.js Line 147 in dcfe0ba
Perhaps we just need to treat that css file as a template? |
We could, but that would still mean a separate tornado handler and prevents using a static server like cdn or nginx from serving the CSS. |
True. How did it work before this with the font paths? Even with the css bundled into the js, the fonts were separate files, right? |
They've always been separate files, yes. I don't follow what you mean though. If you mean before this PR, they were loaded dynamically by webpack using the webpack public path |
In the js, somehow that public path gets set to something like |
Currently it requires people distributing JupyterLab to also have the But your question got me thinking, and maybe it would be best to prebuild the |
Then you have to be careful about deduplicating the packages that need deduplication, like phosphor, etc. |
I will just bundle the |
If that {{}} is in the served JS then it is a bug. It should only be in index.html, from the template file. |
If we build jupyterlab with a previous commit, we see the CSS is all embedded in the JS, for example in the /***/ "S7FT":
/*!***********************************************************************!*\
!*** ../node_modules/@blueprintjs/icons/resources/icons/icons-16.eot ***!
\***********************************************************************/
/*! no static exports found */
/***/ (function(module, exports, __webpack_require__) {
module.exports = __webpack_require__.p + "3326c360137c2d6a6909d0c4303e502a.eot"; I don't understand yet how the server was previously mangling the CSS paths. In the generated |
We were setting the webpack public path dynamically in dev_mode/index.js |
Line 13 in a8e4af9
|
Ah I see, thank you. OK well in my current branch, I can try to bundle the vega dependencies separately and switch back to the old way of loading CSS |
That sounds great. If that doesn't work, then perhaps we revert back to only providing vega 5. |
Looks like we should have read this comment right after the public path setting: Lines 15 to 16 in a8e4af9
|
I tried to help. 😉 |
I think fixing this issue is probably the last breaking thing before the rc. |
I've made #6684 about fixing this problem, so we have a record of the issue. |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related discussion. |
Fixes #6572