dfilatov / vidom

Library to build UI based on virtual DOM
MIT License
415 stars 16 forks source link

Is there a DOM ref issue? #121

Closed ghost closed 9 years ago

ghost commented 9 years ago

I know test cases work just fine, but test the example given in the readme and do an alert on 'this.getDomRef' in the 'onMount'. Is something strange for you?

dfilatov commented 9 years ago

I've tried following:

var InputComponent = vidom.createComponent({
        onRender : function() {
            return vidom.createNode('div')
                .attrs({ className : 'input' })
                .children(
                    this.setDomRef( // add "control" reference to get corresponding DOM node further
                        'control',
                        vidom.createNode('input')
                            .attrs({
                                type : 'text',
                                className : 'input__control',
                                onFocus : this.onFocus.bind(this)
                            })));
        },

        onMount : function() {
            this.getDomRef('control').focus(); // use reference got in onRender()
        },

        onUnmount : function() {
        },

        onFocus : function() {
        }
    });

vidom.mountToDom(
    document.body,
    createNode(InputComponent));

Everything worked as expected.

ghost commented 9 years ago

Ok. I see you don't want to fix this. Anyway so other people know what to avoid. If I try to mount to a existing HTML div tag, I get TypeError: this.getDomRef(...).focus is not a function else if I try to mount to document or document.body, I get this: TypeError: node is undefined. Last message happened when it try to get ID for the event handler.

If I do an alert() on this.getDomRef it tells me that is a text.

dfilatov commented 9 years ago

I can't fix if I don't understand what I should fix. It's realy hard to get from you non-working code. CODE, not just words that something is not working. Because I can't always guess what code you try to run.

ghost commented 9 years ago

Ok. If I do this:

var InputComponent = vidom.createComponent({
        onRender : function() {
            return vidom.createNode('div')
                .attrs({ className : 'input' })
                .children(
                    this.setDomRef( // add "control" reference to get corresponding DOM node further
                        'control',
                        vidom.createNode('input')
                            .attrs({
                                type : 'text',
                                className : 'input__control',
                                onFocus : this.onFocus.bind(this)
                            })));
        },

        onMount : function() {
            this.getDomRef('control').focus(); // use reference got in onRender()
        },

        onUnmount : function() {
        },

        onFocus : function() {
        }
    });

vidom.mountToDom(
    document.body,
    createNode(InputComponent));

Console.log TypeError: tree.adoptDom is not a function

If I change to this

vidom.mountToDom(
    document.body,
    vidom.createNode(InputComponent)); 

Console.log TypeError: node is undefined

or if I try this:

var foo = new InputComponent;

vidom.mountToDom(
    document.body,
    foo);

Console.log TypeError: node is undefined

If i change the whole damn thing and try to do it on a ID tag

vidom.mountToDom(
    document.getElementById("fooBar"),
    vidom.createNode(InputComponent));

Console.log TypeError: this.getDomRef(...).focus is not a function

If i now try to do alert() on this.getDomRef() it tells me that it's a text. I guess text node??

dfilatov commented 9 years ago

Please try to use clean checkout, without any your modifications, I think you're doing something wrong.

And give me all files you run, not only this snippet.

ghost commented 9 years ago

You asked about CODE. The files I'm using are on the github repo. No modifications.

Updated. I found solution on why you can't mount on a ID. You can't use wrapped nodes.

Doesn't work: <div id="fooBar"><div>Hello!</div></div>

Works: <div id="fooBar"></div>

Why it doesn't work on document and document.body I haven't figured out yet.

ghost commented 9 years ago

I think I can't do this on document.body or document because the event handler thing can't set an ID on it? From there I get this error: TypeError: node is undefined

dfilatov commented 9 years ago

You shouldn't mount virtual tree to non-empty and mismatched DOM tree.

dfilatov commented 9 years ago

I think I can't do this on document.body or document because the event handler thing can't set an ID on it?

Event handler is attached to input node.

ghost commented 9 years ago

I know that, so what I did, is simply copying your code form the second post. And there you are attaching it to document.body.

    document.body,
    createNode(InputComponent));

But what about that wrapped node issue

dfilatov commented 9 years ago

My document body is empty at that moment.

ghost commented 9 years ago

My HTML markup

<!DOCTYPE html>
<html>
<head>
    <title>Foo!</title>
</head>
<body>

<div id="fooBar"></div>

</body>
</html>
ghost commented 9 years ago

But try to mount on wrapped nodes as I wrote above. What happend in your end?

dfilatov commented 9 years ago

Your document body isn't empty, you can't mount arbitratry virtual tree to it.

dfilatov commented 9 years ago

But try to mount on wrapped nodes as I wrote above. What happend in your end?

You can't mount mismatched virtual tree to existing dom tree.

ghost commented 9 years ago

Wrapped nodes as I had are valid W3C markup, and the ID are found. How can that be a mismatch?

Anyhow. It will still not find a node if I have this markup:

<!DOCTYPE html>
<html>
<head>
    <title>Foo!</title>
</head>
<body>
</body>
</html>
ghost commented 9 years ago

Now I tried various examples from here. http://www.w3.org/QA/2002/04/valid-dtd-list.html

Same results

dfilatov commented 9 years ago

DTD isn't related with this at all.

ghost commented 9 years ago

I know, I just set up various valid HTML pages and tried the code. With body and empty body. Wrapped tags and non wrapped tags. And so on.

Same results.

dfilatov commented 9 years ago

Seems I understand. You include your javascript before <body/>, don't you? document.body doesn't exist at that moment.

ghost commented 9 years ago

No. I attached the javascript actually in the bottom of the webpage.

On all other virtual DOM scripts I have tried, you can mount without a problem. Even like this: <div id="fooBar"><div>Hello!</div></div> when the JS code are inside the body and outside the body.

But here you can't.

I'm investigating on this, and first off I think the event listener need to be attached to document at the first place, and buble up from there. You don't do that in your code it seems. Or maybe I'm wrong.

So when you attach a node and it get triggered, you find it and attach a handler.

https://github.com/Raynos/mercury/blob/master/dist/mercury.js#L305-L344

You see it better what I mean in Trackira. https://github.com/trackira/trackira/blob/master/src/Element/events/shared/globalEventListener.js https://github.com/trackira/trackira/blob/master/src/Element/events/setup.js

Both of this has a globalEventlistener. A set of events get attached to the document and you can listenTo and notListenTo

If you do similar, I think that can solve the event issue, but still the node issue left.

ghost commented 9 years ago

Both Mercury and Trackira got a set of common Events that get attached: https://github.com/Raynos/mercury/blob/master/dist/mercury.js#L460

dfilatov commented 9 years ago

getDomRef isn't related to events at all.

ghost commented 9 years ago

I was now thinking about this: TypeError: node is undefined as I get because of the focus event. But if I comment that out, it will show me the `getDomRef error

But can you in reality set a unique number on document itself or document.body? That is what you are trying to do and cause the error. In the other solutions I linked to, it get pushed to an array without a unique identifier.

dfilatov commented 9 years ago

I think it's because <body/> isn't empty. Even document.body has empty text node (

ghost commented 9 years ago

Let me see your HTML markup and I will test it. But as it is with other VD implementations, body doesn't need to be empty.

dfilatov commented 9 years ago

What do other VD implementation if you try to insert mismatched virtual tree to DOM tree?

ghost commented 9 years ago

I will test that very soon. here is an working example. I got it to work this way on a ID tag.

<!DOCTYPE html>
<html>
<head>
    <title>Test</title>
<script src="./vidom.js"></script>
</head>
<body>

<div id="fooBar"></div>

</body>
</html>

<script>

var InputComponent = vidom.createComponent({
        onRender : function() {
            return vidom.createNode('div')
                .attrs({ className : 'input' })
                .children(
                    this.setDomRef( // add "control" reference to get corresponding DOM node further
                        'control',
                        vidom.createNode('input')
                            .attrs({
                                type : 'text',
                                className : 'input__control',
                                onFocus : this.onFocus.bind(this)
                            })));
        },

        onMount : function() {
            this.getDomRef('control').focus(); // use reference got in onRender()
        },

        onUnmount : function() {
        },

        onFocus : function() {
        }
    });

vidom.mountToDom(
    document.getElementById("fooBar"),
    vidom.createNode(InputComponent));
</script>
ghost commented 9 years ago

Answer. What is not working with your solution regarding HTML, are working just fine with this libraries:

You don't need to have a empty body either. And they seem to work with wrapped nodes as well.

dfilatov commented 9 years ago

I'll ask once more: What do other VD implementation if you try to insert mismatched virtual tree to DOM tree? Do they clean this dom node before mount?

ghost commented 9 years ago

I will check the source codes.

ghost commented 9 years ago

In Trackira it uses querySelector to find the DOM node, then if the node are found, it goes on to mount the tree. If the tree are mounted already, it unmount the tree first. it seems IF the given HTML CSS selector doesn't exist in the tree, it will not mount.

https://github.com/trackira/trackira/blob/master/src/Tree/prototype/apply.js#L14-L40

I'm checking citoJS now....

ghost commented 9 years ago

citoJS doesn't have a mount. Only a single DOM element that you need to use querySelector first and append on that selector. All children etc, are then attached on this node with appendChild. But it works.

https://github.com/joelrich/citojs/blob/master/dist/cito.js#L1245-L1248

I checked two now. Should be enough.

dfilatov commented 9 years ago

You haven't understood my question. How do trackira and citoJS adopt existing DOM tree to mounted virtual tree if they are mismatched?

ghost commented 9 years ago

SSR? OK. citoJS doesn't have this feature. Trackira has. And it seems to be some complex shit. But seems like he ignore the node and let it die silently. https://github.com/trackira/trackira/blob/master/src/Element/prototype/append.js#L12-L13

and here is a comment:

https://github.com/trackira/trackira/blob/master/src/core/append.js#L13-L18

dfilatov commented 9 years ago

I'll add special case for such empty nodes as body.

ghost commented 9 years ago

And what about the focus event?

dfilatov commented 9 years ago

Focus event just works.

ghost commented 9 years ago

I see. And wrapped nodes?

But can't you just talk with @kflash - author of Trackira about this and maybe he could help?

dfilatov commented 9 years ago

What do you mean by "wrapped nodes"? If you try to mount virtual tree to mismatched dom tree you are doing something wrong, you shouldn't do such things, they don't have any sense.

ghost commented 9 years ago

This is legal HTML markup: <div id="fooBar"><div>Hello!</div></div> but if I try to grab fooBar and mount on it, the ref will throw errors. However. I I remove the div tag in the middle it works.

dfilatov commented 9 years ago

Because you try to mount mismatched virtual tree. If you try to mount to the following dom <div id="fooBar"><div class="input"><input type="text" class="input__control"/></div></div>, everything will be fine.

ghost commented 9 years ago

Ah. Got it to work now that one. I didn¨t know it was looking for the className. I thought that was something was set when you created a VD element.

ghost commented 9 years ago

Hi, @dfilatov. What is this all about? Seems like @rickardjanson tagged me in this thread and I now receive messages from it. There is a reason I reported Mr. Janson to the Github staff earlier today after receiving 15 emails in 1 hour about server side rendring.

ghost commented 9 years ago

ups...

dfilatov commented 9 years ago

Hi, @Kflash. Sorry, but I did nothing to disturb you.

ghost commented 9 years ago

@dfilatov I understand who it is... I just got fucked up earlier today because of him. Truth is. I mostly don't have time for this things. I have my own projects and my own life. If there is any serious questions that I can help out with. I can try my best, but be serious about it. Else. Don't tag me or send me thousands of emails. I'm not trying to be rude, but we need to see the reality too.

ghost commented 9 years ago

Sorry! I just tried to help out here!

ghost commented 9 years ago

@rickardjanson To make things clear. @dfilatov code works just fine. There is nothing wrong with the code as I see it. The problem seems to be you!!

You can't do any server side rendring on a invalid markup. You need to have a reference node, if you tend to use that feature. And you can't mount on invalid HTML markup either.

Example if you want the ref to work, you have to set the correct HTML markup first. If not, it wil throws.

You can not attach anything on a document or document.body as you pointed out, if your reference are className : 'input__control'. Simply because either document or document.body does have this HTML markup, so it will fail.

Nothing wrong with his event system either as far as I see. Only excelent good code!!

As long as you have a correct reference to valid HTML markup, it will work right out of the box for you. Else it will not work as you have created an issue about.

What @dfilatov can do, is putting in some code that either throws a warning in the console.log for devs, or simply ignore and do nothing if invalid markup. As you have pointed out I have done in my code.

For correct server side rendring, you also here need to have a valid HTML markup. So maybe time to go back to school, Mr. Janson?

Summary. There are many ways to Rome. And many ways to develop. You can't compare our code like you do either. That will be wrong for both of us. Hope this make sense.