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

@jupyterlab-dev-mode
Copy link

Thanks for making a pull request to JupyterLab!

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

@blink1073
Copy link
Member

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

@saulshanabrook
Copy link
Member Author

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

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

Ah, I see what you mean.

@jasongrout
Copy link
Contributor

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

(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

(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

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

@ian-r-rose
Copy link
Member

Thanks @saulshanabrook!

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

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

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 June 24, 2019 16:35
@vidartf
Copy link
Member

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 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
@jasongrout jasongrout added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pkg:vega status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. tag:Build System
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't extract css Ensure only vega-embed version
5 participants