Implement `configure dom functionality

Kal-Aster commented 3 years ago

Here the PR discussed in #29

Actually I can't figure out why jsdom is returning a wrong output... The test fails because of that, but trying to create the same html manually it works fine, so I can't understand why via riot rendering it goes wrong.

The "manual" code is:

    domGlobals.create()
    const html = document.createElement('html')
    const head = html.appendChild(document.createElement('head'))
    const title = head.appendChild(document.createElement('title'))
    title.innerHTML = 'hello'
    const meta = head.appendChild(document.createElement('meta'))
    meta.setAttribute('content', 'a description')
    meta.setAttribute('name', 'description')
    const body = html.appendChild(document.createElement('body'))
    body.appendChild(document.createElement('p')).innerHTML = 'hello'
    body.appendChild(document.createElement('script')).setAttribute('src', 'https://path.to/riot.js')
    html.outerHTML === '<html><head><title>hello</title><meta content="a description" name="description"></head><body><p>hello</p><script src="https://path.to/riot.js"></script></body></html>')
    domGlobals.clear()

but jsdom via ssr outputs '<html><title>hello</title><meta name="description" content="a description"><p>hello</p><script src="https://path.to/riot.js"></script></html>' (so without head and body)

Any help figuring this out?

GianlucaGuarini commented 3 years ago

Please let me to understand the problem:

You opened https://github.com/riot/ssr/issues/29 because linkedom couldn't handle your use case
We came out with the idea of patching @riotjs/ssr to handle also your use case (plugging in any node DOM library)
You made a patch that doesn't solve the issue we were trying to fix

Although I think this feature can be a good idea for @riotjs/ssr (this patch needs still a bit of work), I guess your use case might hide other problems not direcly linked with the tools you are using. Could you please prepare a simple demo to show what you are trying to achieve?

Kal-Aster commented 3 years ago

Well, the issue came from the fact that the style object in the HTMLElements is missing the setProperty method, and with that many others. I prepared a sample. I didn't know how I could share it, so I prepared a repo, if you want to run it yourself. However, running the compiled script, it results in the following error:

...\dist\index.js:124390
                this.root.style.setProperty("--color", "red");
                                ^

TypeError: this.root.style.setProperty is not a function
    at Object.onMounted (...\dist\index.js:124390:26)
    at Object.mount (...\dist\index.js:123921:28)    
    at Object.mount (...\dist\index.js:123764:27)    
    at Object.update (...\dist\index.js:122956:17)   
    at Object.mount (...\dist\index.js:122942:18)    
    at ...\dist\index.js:123179:35
    at Array.forEach (<anonymous>)
    at Object.mount (...\dist\index.js:123179:20)    
    at Object.mount (...\dist\index.js:123920:34)    
    at Object.mount (...\dist\index.js:123764:27)

Of course, this is just a silly use case whose only merit is to have shown the issue, because it can be "solved" setting the setProperty method in the CSSStyleDeclaration class to whatever, even just () => {}. But, as I said, the issue is there and, as you said, "I think this feature can be a good idea for @riotjs/ssr"

The problem with this PR, is just that I can't understand the behavior emerged using JSDOM for rendering, hence I asked for help.

Kal-Aster commented 3 years ago

I found a better way to show the example here. BTW, I hope I can take the time to find a solution myself this weekend.

GianlucaGuarini commented 3 years ago

@Kal-Aster on the server there is no need to make calls to element.style.setProperty. CSS should be serialized as string and sent to the initial HTML. You can easily do your setProperty calls only when the Riot.js components are rendered via Browser.

GianlucaGuarini commented 2 years ago

@Kal-Aster I might consider adding this feature only if it's really needed. At moment your use case seem to be more a mistake than something that requires a linkedom replacement. Thank you for your time in making this patch.

riot / ssr

Implement `configure dom functionality #30