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

Add jupyter-renderers #3157

Merged
merged 16 commits into from Nov 7, 2017
Merged

Add jupyter-renderers #3157

merged 16 commits into from Nov 7, 2017

Conversation

@gnestor
Copy link
Contributor

@gnestor gnestor commented Oct 26, 2017

Closes #2587

There's consensus about moving over json-extension and vdom-extension. I also think that geojson-extension should be bundled because ipython now provides IPython.display.GeoJSON.

I’m running into some new TS compile errors:

../json-extension/src/component.tsx(144,36): error TS2339: Property 'includes' does not exist on type 'string'.
../json-extension/src/component.tsx(172,31): error TS2339: Property 'includes' does not exist on type 'string'.
../json-extension/src/component.tsx(190,15): error TS2339: Property 'includes' does not exist on type 'string'.
../json-extension/src/component.tsx(219,14): error TS2339: Property 'includes' does not exist on type 'string'.
../vdom-extension/src/index.tsx(16,18): error TS7016: Could not find a declaration file for module '@nteract/transform-vdom'. '/Users/grant/Sites/jupyter/jupyterlab/node_modules/@nteract/transform-vdom/lib/index.js' implicitly has an 'any' type.
  Try `npm install @types/@nteract/transform-vdom` if it exists or add a new declaration (.d.ts) file containing `declare module '@nteract/transform-vdom';`
@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 26, 2017

This fixes the vdom error:

diff --git a/packages/all-packages/src/typings.d.ts b/packages/all-packages/src/typings.d.ts
index 774f20cfe..5a3fe00a7 100644
--- a/packages/all-packages/src/typings.d.ts
+++ b/packages/all-packages/src/typings.d.ts
@@ -6,4 +6,5 @@
 /// <reference path="../../coreutils/typings/url-parse/url-parse.d.ts"/>
 /// <reference path="../../rendermime/typings/ansi_up/ansi_up.d.ts"/>
 /// <reference path="../../terminal/typings/xterm/xterm.d.ts"/>
+/// <reference path="../../vdom-extension/src/transform-vdom.d.ts"/>
 /// <reference path="../../vega2-extension/typings/vega-embed/vega-embed.d.ts"/>

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 26, 2017

Other changes (requires a new npm install to pick up symlinks):

diff --git a/packages/all-packages/tsconfig.json b/packages/all-packages/tsconfig.json
index e175b56f9..5817b75ad 100644
--- a/packages/all-packages/tsconfig.json
+++ b/packages/all-packages/tsconfig.json
@@ -11,6 +11,7 @@
     "lib": [
       "ES5",
       "ES2015.Collection",
+      "ES2015.Core",
       "ES2015.Promise",
       "DOM"
     ],
diff --git a/packages/json-extension/package.json b/packages/json-extension/package.json
index b5012b542..6980347df 100644
--- a/packages/json-extension/package.json
+++ b/packages/json-extension/package.json
@@ -16,7 +16,7 @@
     "mimeExtension": true
   },
   "dependencies": {
-    "@jupyterlab/rendermime-interfaces": "^0.4.0",
+    "@jupyterlab/rendermime-interfaces": "^0.4.1",
     "@phosphor/coreutils": "^1.2.0",
     "@phosphor/widgets": "^1.3.0",
     "react": "~16.0.0",

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 26, 2017

The other two need their rendermime-interfaces deps updated as well.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 26, 2017

Snap crackle pop baby:

image

cc @rgbkrk

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Oct 26, 2017

Boom baby!! 👊

image


```bash
# Clone the repo to your local environment
git clone https://github.com/jupyterlab/jupyter-renderers.git
Copy link
Member

@blink1073 blink1073 Oct 26, 2017

These repo links will need to be updated in all README and package.json files.

@@ -0,0 +1,151 @@
{
Copy link
Member

@blink1073 blink1073 Oct 26, 2017

this snuck in here

@gnestor gnestor force-pushed the add-jupyter-renderers branch from cdc96aa to df10472 Oct 26, 2017
@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 26, 2017

For reference, bundle sizes are:

  • 8.41 MB with all three renderers
  • 7.99 MB without the geojson renderer
  • 7.66 MB prior to this PR

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Oct 26, 2017

Any thoughts about how to cut down the geojson-extension footprint? I assume leaflet is a relatively large library?

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 26, 2017

Hmm...

image

image

image

image

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 26, 2017

We could load leaflet asynchronously using import() when the class is instantiated.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 26, 2017

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 26, 2017

Actually, since we're compiling to ES5, it would have to be require.ensure(). We should also name it to distinguish it from the one we're using for CodeMirror modes (which should also be using a named require.ensure).

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 26, 2017

For the above errors, we need to check for the existence of the layer before using it in onAfterAttach/onResize, because it doesn't exist until the first time we render, which doesn't come until after the document loaded from disk.

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Oct 26, 2017

Try git reseting to this branch. I pushed the "Fix geojson-extension document rendering" commit that should fix this.

@rgbkrk
Copy link
Member

@rgbkrk rgbkrk commented Oct 26, 2017

Maybe keep the leaflet one out of this PR? Just do JSON + vdom?

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 27, 2017

Did some experimenting:

  • If we use require.ensure we lose the ability to get type information about leaflet because require() has the signature (id: string): any;.
  • If we use import() we get the type information but it gets transpiled to Promise.resolve().then(function () { return require( because we are targeting ES5. There is an npm package that provides import() if TS would let us use it in ES5.
  • We could leave it as a synchronous import and split it into a different bundle using an explicit vendor chunk.

My vote is to keep it, as a vendor chunk.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 27, 2017

If we agree on the vendor chunk approach, we can do that in a follow up PR that also handles React, CodeMirror, and xterm.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 27, 2017

The Travis failure is legit, we need to add @phosphor/messaging@^1.2.1 to geojson-extension dependencies.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 27, 2017

Here is a version of the geo json renderer the lazily loads the leaflet JS and CSS and supports proper typing. https://gist.github.com/blink1073/fa5385530e7199b5485c1ee95344406d

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 27, 2017

Note: we can't use the import foo = require('foo') syntax outside of the top level of the module: "error TS1232: An import declaration can only be used in a namespace or module."

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Oct 27, 2017

Nice, @blink1073!! I assume that I should use this approach with plotly-extension vs. adding a script tag.

I'm getting the following linter warnings:

Cannot find name 'require'.	136:8
Cannot find name 'NodeRequire'.	136:82

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 27, 2017

You need to set types: ["node"] in tsconfig.json.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 27, 2017

Not sure if that will work for plotly because the top level stuff is UMD.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Nov 1, 2017

@gnestor, shall we leave GeoJSON out for now and merge the other two?

@rgbkrk
Copy link
Member

@rgbkrk rgbkrk commented Nov 1, 2017

whispers yes

I'm hoping to make another vdom (for Python) release in the next day or so, I'd love to include mention of the latest JupyterLab support. 😄

@blink1073
Copy link
Member

@blink1073 blink1073 commented Nov 1, 2017

We had planned to leave it in master for a bit, so it won't be released this week either way.

@rgbkrk
Copy link
Member

@rgbkrk rgbkrk commented Nov 1, 2017

All good, we'll always have new releases. 😅

@gnestor gnestor force-pushed the add-jupyter-renderers branch from d929ce2 to 449e0f1 Nov 3, 2017
@blink1073 blink1073 force-pushed the add-jupyter-renderers branch from 449e0f1 to 748814b Nov 7, 2017
@blink1073 blink1073 changed the title [WIP] Add jupyter-renderers Add jupyter-renderers Nov 7, 2017
@blink1073 blink1073 added this to the Beta milestone Nov 7, 2017
afshin
afshin approved these changes Nov 7, 2017
Copy link
Member

@afshin afshin left a comment

This is 🔥.

👍

@afshin afshin merged commit 33d812b into jupyterlab:master Nov 7, 2017
2 checks passed
@gnestor gnestor deleted the add-jupyter-renderers branch Nov 7, 2017
@rgbkrk
Copy link
Member

@rgbkrk rgbkrk commented Nov 7, 2017

Woohoo!

@blink1073 blink1073 mentioned this pull request Nov 8, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Aug 9, 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.

4 participants