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

Ship vega4-extension by default #6591

Merged
merged 17 commits into from Jun 21, 2019
Merged

Ship vega4-extension by default #6591

merged 17 commits into from Jun 21, 2019

Conversation

@domoritz
Copy link
Member

@domoritz domoritz commented Jun 17, 2019

Fixes #6572

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Jun 17, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jun 17, 2019

I think I've gotten myself a bit confused. If both extensions only import vega-embed, and the same version of that library is used, then how do they determine which versions of vega/vega-lite should be used?

@domoritz
Copy link
Member Author

@domoritz domoritz commented Jun 17, 2019

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.

@domoritz
Copy link
Member Author

@domoritz domoritz commented Jun 17, 2019

Does Jupyterlab allow multiple versions of the same package at the same time? If not, we may need an alias release vega4 or something.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Jun 18, 2019

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 doesn't seem to have a way to know which version of vega and vega-lite to use.

@domoritz
Copy link
Member Author

@domoritz domoritz commented Jun 19, 2019

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.

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Jun 19, 2019

@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.

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Jun 19, 2019

@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 ^1.7.0 which means 1.10.0 is allowed and I think that version has breaking changes? It's giving me some typescript errors trying to build with vega-lite 2.6.0 and that version of vega-util:

../../node_modules/vega-lite/build/src/log.d.ts:23:5 - error TS2416: Property 'level' in type 'LocalLogger' is not assignable to the same property in base type 'LoggerInterface'.
  Type '() => this' is not assignable to type '(_: number) => number | LoggerInterface'.
    Type 'this' is not assignable to type 'number | LoggerInterface'.
      Type 'LocalLogger' is not assignable to type 'number | LoggerInterface'.
        Type 'LocalLogger' is not assignable to type 'LoggerInterface'.
          Type 'this' is not assignable to type 'LoggerInterface'.
            Property 'error' is missing in type 'LocalLogger' but required in type 'LoggerInterface'.

23     level(): this;
       ~~~~~

  ../../node_modules/vega-util/index.d.ts:140:3
    140   error(...args: any[]): LoggerInterface;
          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    'error' is declared here.

../../node_modules/vega-lite/build/src/log.d.ts:24:5 - error TS2416: Property 'warn' in type 'LocalLogger' is not assignable to the same property in base type 'LoggerInterface'.
  Type '(...args: any[]) => this' is not assignable to type '(...args: any[]) => LoggerInterface'.
    Type 'this' is not assignable to type 'LoggerInterface'.

24     warn(...args: any[]): this;
       ~~~~

../../node_modules/vega-lite/build/src/log.d.ts:25:5 - error TS2416: Property 'info' in type 'LocalLogger' is not assignable to the same property in base type 'LoggerInterface'.
  Type '(...args: any[]) => this' is not assignable to type '(...args: any[]) => LoggerInterface'.
    Type 'this' is not assignable to type 'LoggerInterface'.

25     info(...args: any[]): this;
       ~~~~

../../node_modules/vega-lite/build/src/log.d.ts:26:5 - error TS2416: Property 'debug' in type 'LocalLogger' is not assignable to the same property in base type 'LoggerInterface'.
  Type '(...args: any[]) => this' is not assignable to type '(...args: any[]) => LoggerInterface'.
    Type 'this' is not assignable to type 'LoggerInterface'.

26     debug(...args: any[]): this;
       ~~~~~

In the mentioned log.d.ts, we have:

export { LoggerInterface } from 'vega-util';
/**
 * Logger tool for checking if the code throws correct warning
 */
export declare class LocalLogger implements LoggerInterface {
    warns: any[];
    infos: any[];
    debugs: any[];
    level(): this;
    warn(...args: any[]): this;
    info(...args: any[]): this;
    debug(...args: any[]): this;
}

which looks like it is missing an error method?

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Jun 20, 2019

I opened a PR against this branch which gets things working locally for me: domoritz#1

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Jun 20, 2019

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jun 20, 2019

@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.

@ellisonbg ellisonbg added this to the 1.0 milestone Jun 20, 2019
@domoritz
Copy link
Member Author

@domoritz domoritz commented Jun 20, 2019

#6591 (comment) should be fixed in Vega-Lite 2.7, which I just released.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jun 21, 2019

The extractor supports a function for public path, that could be made to work: https://github.com/webpack-contrib/mini-css-extract-plugin#publicpath

@vidartf
Copy link
Member

@vidartf vidartf commented Jun 21, 2019

@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?

@vidartf
Copy link
Member

@vidartf vidartf commented Jun 21, 2019

(Trying to ensure we stick to our promise that core extensions are not "special" in any way)

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 21, 2019

The extractor supports a function for public path, that could be made to work: https://github.com/webpack-contrib/mini-css-extract-plugin#publicpath

Is the idea here that the public path function relies on a base url from the page config?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 21, 2019

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

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 21, 2019

@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?

IIRC, all of the buildutils here are integrity checks on our own packages, not necessary things that all packages need.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jun 21, 2019

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...

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 21, 2019

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:

@font-face {
  font-family: "Icons16";
  font-weight: normal;
  font-style: normal;
  src: url({{page_config.fullStaticUrl}}/6fdb722c94434811f89a68eafc4531f8.eot?#iefix) format("embedded-opentype"), url({{page_config.fullStaticUrl}}/cd1a26696ebf17a89545a3f9067d7028.woff) format("woff"), url({{page_config.fullStaticUrl}}/5fa1b8f25b4aa8787f70d5b6b7f0b90f.ttf) format("truetype"); }

which is coming from setting the webpack config at

publicPath: '{{page_config.fullStaticUrl}}/',

Perhaps we just need to treat that css file as a template?

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jun 21, 2019

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.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 21, 2019

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?

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jun 21, 2019

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

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 21, 2019

In the js, somehow that public path gets set to something like /lab/static/, i.e., the '{{page_config.fullStaticUrl}}/' is resolved. Do you know who does that resolution?

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Jun 21, 2019

Does all this custom code in buildutils mean that third-party extensions will not be able to accomplish such a side-by-side setup?

Currently it requires people distributing JupyterLab to also have the nohoist argument added to their package.json. Or maybe not require, just that's the only way I got our webpack to pick up things properly. With another build system, it might resolve the paths in a slightly different manner.

But your question got me thinking, and maybe it would be best to prebuild the vega dependencies in each of the extensions into a seperate bundle, then those bundles get published with the packages, so that anyone who installs the extension doesn't have to worry about the dependency resolution.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 21, 2019

then those bundles get published with the packages, so that anyone who installs the extension doesn't have to worry about the dependency resolution.

Then you have to be careful about deduplicating the packages that need deduplication, like phosphor, etc.

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Jun 21, 2019

Then you have to be careful about deduplicating the packages that need deduplication, like phosphor, etc.

I will just bundle the vega-embed import that captures vega-lite and vega as well. Then the main extension will import that, just as it was importing vega-embed, but now it will just be one big JS file.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jun 21, 2019

If that {{}} is in the served JS then it is a bug. It should only be in index.html, from the template file.

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Jun 21, 2019

If we build jupyterlab with a previous commit, we see the CSS is all embedded in the JS, for example in the vendors.js file:

/***/ "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 index.html, I see the base_url being inserted in the page config and to access the favicons, and the page_config.fullStaticUrl being used to get the URL to the JS file. I am trying to understand how that affected the URLs to the fonts.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jun 21, 2019

We were setting the webpack public path dynamically in dev_mode/index.js

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jun 21, 2019

__webpack_public_path__ = PageConfig.getOption('fullStaticUrl') + '/';

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Jun 21, 2019

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

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 21, 2019

That sounds great. If that doesn't work, then perhaps we revert back to only providing vega 5.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 21, 2019

Looks like we should have read this comment right after the public path setting:

// This must be after the public path is set.
// This cannot be extracted because the public path is dynamic.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jun 21, 2019

I tried to help. 😉

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 21, 2019

I think fixing this issue is probably the last breaking thing before the rc.

@jasongrout jasongrout mentioned this pull request Jun 21, 2019
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 21, 2019

I've made #6684 about fixing this problem, so we have a record of the issue.

@lock
Copy link

@lock lock bot commented Aug 6, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

8 participants