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

Prebuild vega utils and revert exporting CSS #6685

Merged
merged 7 commits into from Jun 21, 2019

Conversation

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Jun 21, 2019

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, the vega-embed will be hoisted to the parent directory and webpack might choose to resolve the wrong version of vega-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 relevant vega-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 do build:src. I am open to suggestions on where we should trigger this build.

User-facing changes

None

Backwards-incompatible changes

None

@saulshanabrook saulshanabrook requested a review from jasongrout Jun 21, 2019
@jupyterlab-dev-mode
Copy link

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

Thanks for making a pull request to JupyterLab!

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

@saulshanabrook saulshanabrook removed the request for review from jasongrout Jun 21, 2019
@saulshanabrook saulshanabrook added this to the 1.0 milestone Jun 21, 2019
@blink1073
Copy link
Member

@blink1073 blink1073 commented Jun 21, 2019

Is there any conflicting CSS added by the two Vega packages?

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Jun 21, 2019

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.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jun 21, 2019

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.

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Jun 21, 2019

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 for the 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 lib and that is all it needs, it doesn't actually use the vega-embed package after the extension is built.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jun 21, 2019

Ah, I see what you mean.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 21, 2019

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

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 21, 2019

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

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Jun 21, 2019

(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 :)

Copy link
Member

@ian-r-rose ian-r-rose left a comment

This works great for me, thanks @saulshanabrook

@ian-r-rose
Copy link
Member

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

Thanks @saulshanabrook!

@ian-r-rose ian-r-rose merged commit 2e2591a into jupyterlab:master Jun 21, 2019
9 checks passed
@vidartf
Copy link
Member

@vidartf vidartf commented Jun 22, 2019

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?

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Jun 24, 2019

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 node_moduels folder. This assures that two different subfolders will have independent versions and yarn won't choose one to hoist up.

@saulshanabrook saulshanabrook deleted the prebuild-vega branch Jun 24, 2019
@vidartf
Copy link
Member

@vidartf vidartf commented Jun 24, 2019

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.

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

5 participants