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

create a vdom output type #2840

Closed
wants to merge 4 commits into from
Closed

Conversation

rgbkrk
Copy link
Member

@rgbkrk rgbkrk commented Sep 14, 2017

This implements a renderer for declarative "VDOM" style output (HTML as JSON if you will). It's not even really vdom, as you could do this with document.createElement and enough patience. Maybe sprinkling some morph dom on top. This was first done in nteract and made to be non-specific to React (phosphor should be able to handle it too). Similar PR in nteract: nteract/nteract#1866

application/vdom.v1+json Specification

The top level element must be a VDOMEl as described below:

type VDOMEl = {
  tagName: string, // Must be a valid HTMLElement (web components 👌🏻)
  children: VDOMNode,
  attributes: Object,
};

type VDOMNode = VDOMEl | string | Array<VDOMNode> | null;

Goals

The eventual goal is to be able to do display updates that keep DOM state (only changing the nodes that matter):

whoa emoji time

Here's it working in classic:

screen shot 2017-09-14 at 2 49 22 pm

/cc @willingc @gnestor @lheagy @mariusvniekerk @Carreau @jackparmer

@rgbkrk
Copy link
Member Author

rgbkrk commented Sep 14, 2017

nteractors have started making a vdom library as well which I'd love to collaborate on others with. It allows users to write code that has resulting output like below:

VDOM(
    div([
        h1('Now Incredibly Declarative'),
        p(['Can you believe we wrote ', bold('all this from scratch'), '?']),
        img(src="https://media.giphy.com/media/xUPGcguWZHRC2HyBRS/giphy.gif"),
        p('SO COOL!'),
    ])
)

Now Incredibly Declarative

Can you believe we wrote all this from scratch?

SO COOL!

@rgbkrk
Copy link
Member Author

rgbkrk commented Sep 14, 2017

Where I'm a bit stuck is in figuring out how I could do update displays without nuking the previous element (thereby losing the actual benefit of the stateful display).

@rgbkrk
Copy link
Member Author

rgbkrk commented Sep 14, 2017

By the way, does anyone know if there's a way to get preact as a UMD bundle we can work with require.js here rather than the assumed global that is used in other parts of the code here?

@rgbkrk rgbkrk changed the title [WIP] create a vdom output type create a vdom output type Sep 14, 2017
@rgbkrk
Copy link
Member Author

rgbkrk commented Sep 14, 2017

@minrk can you think of a good strategy for how I could make update less destructive? I was confused here about the difference between toinsert and element. It looks like element is tracked, and I could reuse it for preact.render.

@takluyver
Copy link
Member

Awesome!

@minrk
Copy link
Member

minrk commented Sep 18, 2017

I was confused here about the difference between toinsert and element

Generally toinsert refers to an element that's specifically not on the page yet. If you have a good idea for how to update elements in-place, that would be cool. The way it is right now, we do it in two steps:

  1. create a new output
  2. replace the original output via jQuery after everything else is done

I don't see an obvious way to handle the update-in-place, other than to possibly push down the possible replacement logic into each append_foo method, rather than letting them assume they always populate a new element, as they do now.

@rgbkrk
Copy link
Member Author

rgbkrk commented Sep 18, 2017

other than to possibly push down the possible replacement logic into each append_foo method, rather than letting them assume they always populate a new element, as they do now

Ok, at first I worried about doing that. Perhaps that's ok though. I could end up making updates for the other mimetypes smoother as a result.

@minrk
Copy link
Member

minrk commented Sep 19, 2017

I think it's fine if you figure out a way to make it work.

@rgbkrk
Copy link
Member Author

rgbkrk commented Sep 25, 2017

As requested by @mpacer, here's a GIF showing what happens when the update happens to the VDOM output.

wipeout

I'd actually love to get this in as is then work on the update semantics later so that I'm not changing any base APIs within the outputs.

@rgbkrk
Copy link
Member Author

rgbkrk commented Sep 25, 2017

FAIL ".CodeMirror-code" still did not exist in 10000ms
#    type: uncaughtError
#    file: /home/travis/build/jupyter/notebook/notebook/tests/base/utils.js
#    error: ".CodeMirror-code" still did not exist in 10000ms
#    stack: not provided

Is this normal?

@rgbkrk
Copy link
Member Author

rgbkrk commented Sep 25, 2017

Looks like this is happening on master too:

 {master %=} kylek@nfml-83G ~/code/src/github.com/jupyter/notebook$ python -m notebook.jstest notebook

Test group: notebook
Running tests with notebook directory '/var/folders/kd/cylz4mhs1_9cpsjh0_c_gzfr0000gn/T/tmp_YfpvE'
Test file: /Users/kylek/code/src/github.com/jupyter/notebook/notebook/tests/notebook/attachments.js
Page Error
  Message:   Error: Script error for "components/moment/locale/en-us"
             http://requirejs.org/docs/errors.html#scripterror
  Call stack:
    line 140 of static/components/requirejs/require.js in defaultOnError
    line 544 of static/components/requirejs/require.js in onError
    line 1732 of static/components/requirejs/require.js in onScriptError
Timeout for http://localhost:8888/a@b/
Is the notebook server running?
FAIL ".CodeMirror-code" still did not exist in 10000ms
#    type: uncaughtError
#    file: /Users/kylek/code/src/github.com/jupyter/notebook/notebook/tests/notebook/attachments.js
#    error: ".CodeMirror-code" still did not exist in 10000ms

#    stack: not provided

@rgbkrk
Copy link
Member Author

rgbkrk commented Sep 25, 2017

This is looking like components/moment/locale/en-us is missing:

$ ls notebook/static/components/moment/locale/en-*
notebook/static/components/moment/locale/en-au.js notebook/static/components/moment/locale/en-ca.js notebook/static/components/moment/locale/en-gb.js

Fixed in #2866.

@rgbkrk
Copy link
Member Author

rgbkrk commented Sep 26, 2017

Fun fact time! It turns out you don't have to do camelCased property names for CSS. It will accept either of them if they match the CSS name:

screen shot 2017-09-25 at 9 31 23 pm

@rgbkrk
Copy link
Member Author

rgbkrk commented Sep 28, 2017

If possible, I'd like to merge this one as is and not worry about the display updates portion of this as it works just not as well as it could.

@gnestor
Copy link
Contributor

gnestor commented Sep 28, 2017

@rgbkrk Sorry I haven't had a chance to review this yet! If you can resolve the Travis issue, then feel free to merge. Otherwise, I will have a look at it tonight 👍

@rgbkrk
Copy link
Member Author

rgbkrk commented Sep 28, 2017

Yeah I have no idea how to resolve the issue on Travis. I'm getting a timeout error for the codemirror cell.

@gnestor
Copy link
Contributor

gnestor commented Sep 29, 2017

I can't figure out this Travis issue, but I can reproduce it by running the tests locally...

@minrk Any idea what might be going on here? https://travis-ci.org/jupyter/notebook/jobs/279802361#L816

@rgbkrk
Copy link
Member Author

rgbkrk commented Sep 29, 2017

I have this sneaky feeling that preact isn't defined on the page by the time this runs and it is currently bombing out. Perhaps the precursor to this PR should be to get react from bower (and trim out the current preact usage), then I'll have this PR do the right thing.

@Madhu94
Copy link
Contributor

Madhu94 commented Oct 6, 2018

I hope this hasn't been dropped. What needs to be done to move this along ? @rgbkrk @gnestor

@rgbkrk
Copy link
Member Author

rgbkrk commented Oct 7, 2018

The main blocker is the test suite, which has been a work in progress to migrate off of the old slimerjs / phantomjs setup. cc @takluyver

@rgbkrk
Copy link
Member Author

rgbkrk commented Oct 7, 2018

At least to me, I'm dropping this PR for the time being pending classic notebook being brought to the future with npm packages. As it stands, most development is invested in all the newer frontends whether nteract or jupyterlab (or Hydrogen, Neutron, etc.). Totally cool if you bring it forward. 😄

Looking at the commits, I'd prefer to have this package be based on @nteract/transform-vdom.

@blink1073
Copy link
Contributor

Closing, since we ended up not switching to npm packages. Thanks for experimenting with this @rgbkrk!

@blink1073 blink1073 closed this Jun 3, 2020
@rgbkrk rgbkrk deleted the vdom-output-type branch June 7, 2020 23:03
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants