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
Prebuild vega utils and revert exporting CSS #6685
Conversation
Thanks for making a pull request to JupyterLab! To try out this branch on binder, follow this link: |
Is there any conflicting CSS added by the two Vega packages? |
I don't believe so anymore, because we are now using the same version of vega-embed in each which has the CSS. |
It seems like the no-hoist decision needs to be pushed down to the extension level. We could add a Jupyterlab field for it that gets added to package.json in dev_mode and staging. |
I believe we don't need nohoist now in dev_mode and staging? Because all the vega gets built into one file in the |
Ah, I see what you mean. |
If we are shipping the vega source, we should figure out the license situation for it and all of its dependencies that we are shipping, and note it in our package license. Edit: looks like at least vegalite is BSD for example, but we'd need to make sure the license statement is included in the bundle we are distributing |
(and really we should be making sure this is happening for our python package as well, since we are shipping that gigantic js bundle of the pre-built source...) |
Exactly what I was going to say :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works great for me, thanks @saulshanabrook
Thanks @saulshanabrook! |
I'm not super into the details here, so for future reference: What does the nohoist thing do? Is it for yarn or for webpack? |
It is for yarn. It tells yarn not to "hoist" your dependencies up to the top level, but to instead leave them in the subfolders |
Thanks! I thought yarn would only hoist the package if their semver ranges overlapped? I'll make a note to check carefully the next time I assume that. |
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. |
References
Fixes #6684
Fixes #6675
Code changes
This changes how we package the vega extensions. Instead of having each depend on different versions of vega-lite and vega, we only depend on them as dev dependencies. Then, in the build process we build them into one large JS file and ship that with the libraries. We import that files just like we were importing
vega-embed
, but it has everything it needs in that one file.This makes it easier to install these extensions in a different environment and not have to worry about hoisting. So I was able to remote the noihoist from staging, addressing @vidartf earlier concern about making it harder to build jupyterlab. However, I still need to keep the nohoist in the main repo, so that each extension gets its own version of
vega-embed
and the rest. If I don't have this, thevega-embed
will be hoisted to the parent directory and webpack might choose to resolve the wrong version ofvega-lite
in each package. I didn't test this, but I wasn't sure how it would play out, so I opted for the safer method of not hoisting any of these libraries.This also let me switch both to the newer
vega-embed
version. Since we are building each separately webpack no longer gets confused about what is the relevantvega-lite
version for each.I tried originally to make these builds part of the main webpack build, like we do for themes, but it was trying to build them simultaneously as the main build, and they need to be built before so the main build can resolve its import of the vega packages.
Currently the webpack build runs on the
build
commands in each package, so should be run when we dobuild:src
. I am open to suggestions on where we should trigger this build.User-facing changes
None
Backwards-incompatible changes
None