jorgebucaran / superfine

Absolutely minimal view layer for building web interfaces
https://git.io/super
MIT License
1.56k stars 78 forks source link

V7 and Document Fragments #172

Closed osdevisnot closed 4 years ago

osdevisnot commented 4 years ago

I really love new simplified patch usage and the simplified algorithm.

However, it seems we assume that the node.parentNode always exists in https://github.com/jorgebucaran/superfine/blob/master/src/index.js#L265

This works for the most part, but fails when the provided node is a DocumentFragment, since DocumentFragment won't have a parentNode.

The patch in this case errors out with:

image

To add more context, I have a web-components wrapper based on Superfine, which was invoking the patch in v6 as https://github.com/osdevisnot/supertag/blob/master/src/supertag.ts#L94

With V7, I believe the correct usage would simply be something like:

this[ROOT] = patch(this[ROOT], this.render());

But this fails with above error since this[ROOT] is a DocumentFragment created using

this[ROOT] = this.attachShadow({ mode: 'open' });

Any pointers on how we can use the V7 patch with DocumentFragments? Or am I missing something?

jorgebucaran commented 4 years ago

@osdevisnot What do you suggest?

evatrium commented 4 years ago

I got a Fragment element working and am able to patch and re-patch subsequent updates reliably into the shadowRoot of a web component. Just kind of a hack. Would be awesome to find a better solution.

demo: https://github.com/iosio/x/blob/master/demo/index.js

vdom - modified superfine https://github.com/iosio/x/blob/master/src/vdom.js

web component base - some trickery with conditional mounting point https://github.com/iosio/x/blob/master/src/xelement.js

ken-okabe commented 4 years ago

I've had the same problem when I tried to migrate to V7 with a simple implementation of timer. Screenshot from 2019-09-04 20-38-47

It fails on the re-patch subsequent updates.


I develop unlimitedjs - https://stken2050.github.io/unlimitedjs/ and the "framework" depends on superfne@6.0 so far.

Migrating to v7 does not work.

jorgebucaran commented 4 years ago

@stken2050 If V6 can do it, so should V7. This can be a bug in the VDOM or a user mistake. Can anyone provide a CodePen or CodeSandBox with the error?

@iosio @osdevisnot Ping.

evatrium commented 4 years ago

@jorgebucaran I have created a sample codepen to demonstrate the different ways of rendering superfine to the shadowdom of a web component, in addition, a root Fragment/Host vnode example.

https://codepen.io/iosio/pen/eYOeaYE?editors=1011

click inc to increment the counter

test-1 no go (commented out)- dev tools logs errors test-2 renders - no update - dev tools logs errors test-3 updates successfully test-4 updates successfully and utilizes a document fragment for the vnode, kinda mimicking what stencil.js does. (although with some slight modifications in the superfine code annotated in the left HTML pane)

jorgebucaran commented 4 years ago

@iosio Thank you. I'll check it out. And to clarify, this works in V6, but not V7?

evatrium commented 4 years ago

@jorgebucaran I just recently found out about superfine, I haven't tried V6 yet.

ken-okabe commented 4 years ago

@jorgebucaran

My virtualDOM framework

unlimitedjs = superfine + timeline-monad(minimal FRP)

I have a simple clock app that works fine with every update with superfine6.0.1.

With superfine7.0.0, the first update works, but every subsequent update fails with error.


unlimitedjs with superfine6.0.1 - works https://codepen.io/stken2050/pen/XWrzvmX

unlimitedjs with superfine7.0.0 - errors https://codepen.io/stken2050/pen/OJLOKgr

Screenshot from 2019-09-05 18-33-24

jorgebucaran commented 4 years ago

Thank you both! I'll check this out asap (possibly over the weekend).

ken-okabe commented 4 years ago

@jorgebucaran Thanks! Is it hard to fix?

jorgebucaran commented 4 years ago

@stken2050 Your problem is unrelated to this. Here's a fork of your pen that works with Superfine 7. See V7's release notes for more info.

osdevisnot commented 4 years ago

@jorgebucaran my apologies, got busy with life. I'll try to investigate this tonight and see if I can reach somewhere. If not, I'll at least create a simpler repro.

Thanks for looking into it 😊

ken-okabe commented 4 years ago

@stken2050 Your problem is unrelated to this. Here's a fork of your pen that works with Superfine 7. See V7's release notes for more info.

I see. document.body as the patching div does not work, instead, some element is required.

I think it would be helpful if you add the information since I think many would test this under document.body directly. I've surely read V7's release, but there's no information about that.

Thanks!

jorgebucaran commented 4 years ago

@stken2050 You could use document.body and produce a <body> with your vdom too. The change that you probably missed is that we replace (recycling if possible) your DOM node now, rather than append to it.

I think it would be helpful if you add the information since I think many would test this under document.body directly.

Will do. 👍

osdevisnot commented 4 years ago

So this makes sense, since the new patch tries to replace the root that was supplied. However, it's a bit unconventional to replace rather than append.

It will create problems in following scenarios:

I have another cool project based on superfine. Will try to fix in the fork when I get some time.

jorgebucaran commented 4 years ago

@osdevisnot rendering to fragment root (which is the case for supertag)

  1. What'd need to change in Superfine to support this out of the box?
  2. What'd need to change in Supertag to support this out of the box?
  3. Are you willing to change Supertag?
osdevisnot commented 4 years ago

I can't think of a non hacky solution on supertag level.

For superfine, we need few changes to consider a new node type DOCUMENT_FRAGMENT_NODE ( nodetype = 11).

for patch, you should be able to get away with something like this

export var patch = function(node, vdom) {
  return (
    ((node = patchNode(
      node.parentNode || node.getRootNode(),
      node,
      node.vdom || recycleNode(node),
      vdom
    )).vdom = vdom),
    node
  )
}

the node.getRootNode() part will give you the host node rather than parent (which does not exist).

this however might not work, since the createNode will createElement on document which is a different tree. So parent.insertBefore in patchNode should fail complaining something related to it being a different tree.

I hope this makes sense. FWIW, I don't think any virtual dom library solved this yet.

snabdom for example has this issue still open https://github.com/snabbdom/snabbdom/issues/190 and there are changes being considered to support documentFragments as first class citizens in V2 roadmap (not sure if this is being actively worked on)

jorgebucaran commented 4 years ago

@osdevisnot Can you make a PR? I'd love to discuss this over code.

osdevisnot commented 4 years ago

Do you have local version of tests that's not on GitHub repo? I would want to ensure my changes aren't breaking anything else when I attempt to fix this.

osdevisnot commented 4 years ago

On second thought, my initial assumption about createNode creating createElement on document is incorrect. It works fine.

Also, for shadowRoot, there isn't a way to reliably replace the original node with new node unless the original node has content pseudo-element

I will play around this a bit to see what other options we can consider.