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

fixes #468 - bugfixes for libraries that use other phosphor libs present in JupyterLab #481

Merged
merged 3 commits into from
May 18, 2020

Conversation

timkpaine
Copy link
Member

defines the other phosphor libraries expected to be present in jlab

@timkpaine timkpaine changed the title fixes #468 fixes #468 - bugfixes for libraries that use other phosphor libs present in JupyterLab Nov 24, 2019
@maartenbreddels
Copy link
Member

Note to self: check bundle size increase.

I think if not too big, I'm ok with this.

@jtpio
Copy link
Member

jtpio commented Jan 27, 2020

@timkpaine have you been using this in a custom template in the meantime?

@timkpaine
Copy link
Member Author

@jtpio in a fork, yep. its necessary for any jlab extension that uses phosphor, e.g. https://github.com/finos/perspective

@timkpaine
Copy link
Member Author

timkpaine commented Feb 12, 2020

need to add:

  • @jupyterlab/coreutils
  • @jupyterlab/application
  • @jupyterlab/apputils
  • @jupyterlab/docregistry

@timkpaine
Copy link
Member Author

timkpaine commented Feb 12, 2020

So based on my analysis:

  • Any extension that uses phosphor needs the set of @phosphor extensions present
  • Any extension that includes a mimerenderer will need the @jupyterlab extensions

Because these packages can't be imported standalone e.g. by require, and are present in JLab, extensions that use them will not work with voila until this PR is in. There may be more deps we need to include to enable other more esoteric extensions to run.

@timkpaine
Copy link
Member Author

timkpaine commented Mar 26, 2020

@maartenbreddels bundle size shouldn't increase much since we already have @phosphor/widgets, which already has a dependency on the other phosphor stuff https://github.com/phosphorjs/phosphor/blob/master/packages/widgets/package.json#L45
So we'd just be exporting it to allow other libraries to reuse.

the @jupyterlab/ stuff would seem worth it even with a bundle size increase, since it enables arbitrary mimerenderers to continue to function

@timkpaine
Copy link
Member Author

interested in any thoughts on this, otherwise I'll need to fork soon for some of my PRs.

@SylvainCorlay
Copy link
Member

I am ok with this and am happy to merge now for integration in 0.2.

ping @jtpio

@SylvainCorlay SylvainCorlay merged commit c2df4a8 into voila-dashboards:master May 18, 2020
@timkpaine
Copy link
Member Author

@SylvainCorlay thanks! The side effect of this merge is that any libraries that also have rendermimes will work out of the box (e.g. see the CC'd Plotly issue).

@timkpaine timkpaine deleted the phosphor branch May 18, 2020 21:16
@SylvainCorlay
Copy link
Member

This is great. We are trying to push towards 0.2 but if we cannot, I may backport it into a 0.1.x.

@timkpaine
Copy link
Member Author

@SylvainCorlay the only question is the phosphor/lumino name change, it might be enough to just import as lumino but define on the window as both the lumino and phosphor name (e.g. just copy paste those lines and s/phosphor/lumino).

@jtpio
Copy link
Member

jtpio commented May 19, 2020

Indeed, aliases for lumino might be enough. Or something similar to the way phosphor / lumino is handled in ipywidgets 7.

maartenbreddels added a commit to widgetti/solara that referenced this pull request Dec 15, 2023
This gets rid of React, CodeMirror, blueprint Yjs and other
smaller dependencies that we do not need in solara.

Some of the original dependencies are added back in:
 voila-dashboards/voila#481
maartenbreddels added a commit to widgetti/solara that referenced this pull request Dec 15, 2023
#422)

This gets rid of React, CodeMirror, blueprint Yjs and other
smaller dependencies that we do not need in solara.

Some of the original dependencies are added back in:
 voila-dashboards/voila#481
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants