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

Some questions... #1

Open
leeoniya opened this issue Nov 11, 2016 · 11 comments
Open

Some questions... #1

leeoniya opened this issue Nov 11, 2016 · 11 comments

Comments

@leeoniya
Copy link

Hey @trueadm this is interesting. I have some questions. Haven't tested yet cause on phone...

  • would it be better to use comment nodes since adjacent fragment nodes would not get merged during rendering to and html string (for ssr).
  • can this accomodate fragments-in-fragments? adjacent fragments?

a more useful demo might be

table
  tbody
    tr
    tr
    tr
    tr
  tbody
    tr
    tr
    tr
    tr
  tbody
    tr
    tr
    tr
    tr
  tbody
    tr
    tr
    tr
    tr

where every 2 adjacent tbody elems are a fragment and every 2 adjacent tr elems are a fragment.

this would go a long way towards demonstrating general utility.

@trueadm
Copy link
Owner

trueadm commented Nov 11, 2016

@leeoniya

  • good point regarding comment nodes. I mentioned that in the code comments actually – would be worth while testing out as I believe merging of text nodes is a legacy problem (Firefox, Safari, Edge and Chrome no longer do this).
  • it should totally be possible to nest fragments. I'll get a working example of that actually – maybe I'll add some formal tests to deal with this (using your use-case would be a good win).

On a side note, I'd love some help from you and others if possible on this. I see this being a massive win for the vdom community if this all works well and performantly.

@leeoniya
Copy link
Author

I believe merging of text nodes is a legacy problem

i meant on the server for vtree -> html string output. but i guess this could be hydration-strategy-specific. i'll provide more concrete feedback after testing but my initial feeling is that comment nodes could offer more flexibility.

On a side note, I'd love some help from you and others if possible on this.

certainly.

I see this being a massive win for the vdom community if this all works well and performantly.

indeed it would.

@leeoniya
Copy link
Author

leeoniya commented Nov 11, 2016

I tried the following:

const container = document.getElementById('container1');
const fragment1 = FragmentNode.createFragmentNode();
const fragment2 = FragmentNode.createFragmentNode();

// 1: create a div, with some text and append it to our fragment
const div1 = document.createElement('div');
div1.textContent = 'I am a DIV 1';
fragment1.appendChild(div1);

const div2 = document.createElement('div');
div2.textContent = 'I am a DIV 2';
fragment1.appendChild(div2);

const div3 = document.createElement('div');
div3.textContent = 'I am a DIV 3';
fragment2.appendChild(div3);

const div4 = document.createElement('div');
div4.textContent = 'I am a DIV 4';
fragment2.appendChild(div4);

// append frags
container.appendChild(fragment1);
container.appendChild(fragment2);

// move frag 1 to end
container.appendChild(fragment1);

This worked as expected. but the placeholder comment nodes that represent the fragments seem to be below the children, not above them. Here's what i think domvm would need to make practical use of this:

  • Tag placeholder nodes with from and to props representing the fragment edge indices within the real .parentNode (or better yet have them be the actual edge nodes)..and keep this info up-to-date. This would help reconciliation algos skip over entire ranges during iteration.
  • Allow (optionally) for both fragment edges to be bounded by placeholders. This would aid any reconciliation algos that work bidirectionally (alternating .previousSibling or.nextSibling). Also tag each placeholder to identify it as a startFragment or endFragment bound.
  • Allow (optionally) to tag a specific id to the placeholders (for general use).
  • Allow the content of the placeholder nodes to rendered in a meaningful way:
<!--^#foo|1:3-->
<tr>
<tr>
<tr>
<!--^#bar|5:7-->
<tr>
<tr>
<tr>

^ prefix would mean startFragment; $ = endFragment ala regex. Else it could be more html-like and use a / for endFragment and no prefix for start.

Haven't thought of how nesting would be represented yet WRT child range indication.

@trueadm
Copy link
Owner

trueadm commented Nov 11, 2016

@leeoniya the comment nodes should always be after the children, so it can effectively use insertBefore. You're right in that there could also be a beginning tracking node – worth hacking up. On a side note: I've added a build and some way of running tests, so if you want to contrib to the library let me know and I'll add you :)

TBH: for now I just want a reliable module, performance can be gained later. This package would also easily allow people to nest children without normalising/flattening them – which can be beneficial for those who mix keyed and non keyed children.

@leeoniya
Copy link
Author

On a side note: I've added a build and some way of running tests, so if you want to contrib to the library let me know and I'll add you :)

Thanks, I'll ping you if I want to make it official :)

TBH: for now I just want a reliable module, performance can be gained later.

To be clear, these are not perf optimizations; domvm would likely require from/to range indicators on the placeholders & both start/end placeholders to properly hydrate/SSR and reconcile fragments. The content setting & id tagging are nice-to-haves but cheap and useful.

If the goal of this project is to be useful to vdom libs, i suspect some may require this as well. Otherwise this simply becomes thin sugar for reducing userland DOM ops for vanillajs. /IMHO.

@leeoniya
Copy link
Author

leeoniya commented Nov 11, 2016

Actually, from/to may not be needed if it's cheap to access some placeHolder.firstChild and placeHolder.lastChild or a fuax/fast/plain-array .childNodes. However, I don't see getting around needing placeholders at both edges. I'll shut up for a while and see if I can get something working with and without my changes.

@trueadm
Copy link
Owner

trueadm commented Nov 11, 2016

Having placeholders at both sides would help debugging too, I'm happy to go for it :)

@thysultan
Copy link

thysultan commented Nov 18, 2016

This could be done without MutationObservers but lib code will have to adapt to support it...

You could create an object that has all the usual methods elements have with one extra signature that identifies it as a proxy element when you call .removeChild/appendChild etc...

you can then do a check to see if it's a proxy element, dom elements will return undefined and access of undefined properties on dom objects as fast as js objects, but if it is you will delegate the task to the proxy element, i.e

if (el.isProxy) {
    el.remove();
} else {
   el.parentNode.removeChild(el);
}

where el.remove is just a method that iterates all the fragments children and removes them.

@trueadm
Copy link
Owner

trueadm commented Nov 18, 2016

@thysultan the whole point of this, is that it's not dependent on heuristics in a library. Plus there's far more to it than simply just dealing with removeChild, there are loads of edge cases that the library would then have to handle too – it's not trivial to do.

@thysultan
Copy link

The .remove was just an example that proxies removeChild... the idea was that you would proxy all the (inserBefore, appendChild operations done on it) etc... but yes the caveat is that lib code will have to adapt to it, though if perf is a nightmare with MutationObservers that may be the next best option.

@thysultan
Copy link

Something like...

function createDocumentFragment () {
    var documentFragment = document.createDocumentFragment();
    var firstElement = document.createComment('<>');
    var lastElement = document.createComment('</>');

    documentFragment.appendChild(firstElement);
    documentFragment.appendChild(lastElement);

    var children = [firstElement, lastElement];

    // methods affecting its children
    documentFragment.removeChild = function (node) {
        firstElement.parentNode.removeChild(node);
        children.splice(children.indexOf(node), 1);
    };
    documentFragment.insertBefore = function (before, node) {
        firstElement.parentNode.insertBefore(node, firstElement);
        children.splice(children.indexOf(node)-1, 1);
    };
    documentFragment.appendChild = function (node) {
        firstElement.parentNode.insertBefore(node, lastElement);
        children.splice(children.length-2, 0, node);
    };
    documentFragment.replaceChild = function () {
        firstElement.parentNode.removeChild(node);
        children[children.indexOf(node)] = node;
    };

    // methods affecting its place in the document
    documentFragment.remove = function () {
        proxy('removeChild');
    };
    documentFragment.append = function () {
        proxy('appendChild');
    };
    documentFragment.replace = function () {
        proxy('replaceChild');
    };
    documentFragment.insert = function (node) {
        proxy('insertBefore', node);
    };

    function proxy (method, node) {
        var parentNode = firstElement.parentNode;
        var length = children.length;
        var i = 0;

        if (node) {
            for (; i < length; i++) {
                parentNode[method](children[i], node);
            }
        } else {
            for (; i < length; i++) {
                parentNode[method](children[i]);
            }
        }
    }

    documentFragment.isProxy = true;

    return documentFragment;
}

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

No branches or pull requests

3 participants