Fix element.insertBefore(node, refNode) to accept falsy second parmeter

jsprog commented 3 years ago

Likely other methods are affected by not tested (e.g: element.insertAfter(node, refNode)) https://github.com/c-smile/sciter-js-sdk/blob/main/status.md

I managed to run a build generated by a svelte project after applying some fixes:

Svelte Project

npx degit sveltejs/template my-svelte-project cd my-svelte-project

Open rollup.config.js and uncomment terser plugin (this should help tracking some errors from terminal)

// production && terser(),

Build the project and execute sciter. you also need to fix paths (styles, scripts, etc..) in index.html by removing the leading forward slash

npm run build scapp ./public/index.html

Reported Error:

error:script: TypeError: Node was deleted

After some checks, I discovered that sciter is claiming about invoking the second parameter (reNode) with a 'null' value.

element.insertBefore(node, null) // Accepted by chrome but not sciter
element.insertBefore(node)       // Accepted by sciter but not chrome

I tried to fix Node.prototype.insertBefore but the moment I write Node.prototype, I get this error:

console.log(Node.prototype)

Reported Error:

terminate called after throwing an instance of 'qjs::exception' Aborted (core dumped) npm ERR! code ELIFECYCLE npm ERR! errno 134 npm ERR! svelte-app@1.0.0 sciter: scapp ./public/index.html npm ERR! Exit status 134 npm ERR! npm ERR! Failed at the svelte-app@1.0.0 sciter script. npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

But I managed to apply a temporary fix either by using @rollup/plugin-replace or by invoking some code before executing the script

Method 1 (@rollup/plugin-replace):

// rollup.config.js
replace({
    include: ['node_modules/svelte/internal/**'],
    delimiters: ['', ''],
    'target.insertBefore(node, anchor || null);': 'let sciter_detected = false; try { navigator } catch { sciter_detected = true } ; (!anchor && sciter_detected) ? target.insertBefore(node) : target.insertBefore(node, anchor || null);',
}),

Method 2 (replacing insertBefore for document.body and any other element created at runtime):

// at the top of src/main.js, add these lines
let sciter_detected = false; try { navigator; } catch { sciter_detected = true; }
if (sciter_detected) {
    document.body.__insertBefore = document.body.insertBefore
    document.body.insertBefore = function (node, anchor) {
        const params = [node, ...(anchor ? [anchor] : [])]
        document.body.__insertBefore(...params)
    }

    document.__createElement = document.createElement
    document.createElement = function (...rest) {
        const element = document.__createElement(...rest)
        element.__insertBefore = element.insertBefore
        element.insertBefore = function (node, anchor) {
            const params = [node, ...(anchor ? [anchor] : [])]
            element.__insertBefore(...params)
        }
        return element
    }
}

Sorry for the long text, I only tried to prove to others the possibility to benefit from some wide spread reactive frameworks

c-smile commented 3 years ago

Try v.4.4.5.4 - I've just uploaded it.

These:

element.insertBefore(node, null);
console.log(Node.prototype);

should work.

jsprog commented 3 years ago

Already confirmed that the issues mentioned above got fixed under Windows but not Linux. I can also confirm about some other issues affecting both Windows and Linux:

document.body.style.setProperty is reported as "string" instead of "function"

document.getElementsByTagName isn't available at all


// test code
const el = document.createElement('div')
el.appendChild(document.createTextNode('text...'))

document.body.insertBefore(el, null) // linux -> error:script: TypeError: Node was deleted

console.log(Node.prototype) // linux -> error: terminate called after throwing an instance of 'qjs::exception'

console.log(typeof document.body.style.setProperty) // linux|windows -> reported as "string" instead of "object" document.body.style.setProperty('background-color', 'red') // linux|windows -> error:script: TypeError: not a function

console.log(document.getElementsByClassName) // linux|windows -> undefined console.log(document.getElementsByTagName('head')) // linux|windows -> error:script: TypeError: not a function

c-smile commented 3 years ago

As of getElementsByClassName and other methods ...

Idea of Sciter.JS is to keep implementation as compact as possible. Not all browser's DOM methods will be available but it will always be other means to add them when needed.

For example, this will add getElementsByTagName:

<html>
    <head>
        <title>Test</title>
        <style></style>
        <script>

Element.prototype.getElementsByTagName = function(tag) {
  return this.querySelectorAll(tag);
}

console.log( document.getElementsByTagName("p") );
console.log( document.body.getElementsByTagName("p") );

        </script>
    </head>
    <body>

<p>Veni</p>
<p>Vidi</p>
<p>Vici</p>

    </body>
</html>

and getElementsByClassName is an exercise for the reader.

As of styles, this works:

document.body.style['background-color'] = 'red';

jsprog commented 3 years ago

The problem is that existing libraries are free to use any valid syntax and you can't do anything about this. But not a big problem in this case as the programmer can polyfill them with ease.

But leaving element.style.setProperty as a "string" property instead of "function" or undefined is an issue, unless if you're planning to support it.

c-smile commented 3 years ago

you can't do anything about this

I understand that. Polyfill will be needed for features that are not in Sciter.

For example window.location makes almost no sense in Sciter.JS context. The only thing from it that is used in popular SPA libraries is hashchange event (for routing). And so this needs to be emulated somehow. Yet there are other features similar to this - API in browsers is huge and it is not realistic for such small engine to support all that. And 80% of those APIs are outdated, not used anyway.

Browser must be a universal platform - to run all possible sites. Sciter case is different in this respect - it runs particular application that uses particular features. Yet Sciter internally is modular - if you don't need std module for example you can exclude it from your binary completely.

Some [modular] compatibility JS layer should be designed so applications may include only features they need.

There is a mechanism in place in Sciter and Sciter.JS that allows to bind JS code using CSS declarations:

html {
   prototype: BrowserCompatibility url(scripts/std-compatibility.js);
}
... other stylistic compatibility declarations ...

And that std-compatibility.js may include declarations like Element.prototype.getElementsByTagName above.

In order to hookup all this we will need to include just CSS:

@import url(std-compatibility.css);

Or for a frame

<frame src="std-content.html" content-style="std-compatibility.css" />

c-smile commented 3 years ago

Also I've added element.style.setProperty() and element.style.getPropertyValue(), see

https://github.com/c-smile/sciter-js-sdk/wiki/DOM.Element.Style

jsprog commented 3 years ago

window.location isn't only used by SPA libraries for routing. Even rollup is using window.location.href to construct the uri for Websocket. Likely in the near future, I'll try to add minimal code or use some rollup transformations to support auto reload during development.

What kept me away from Sciter in the previous years was the use of tiscript which prevented me from using reactive frameworks like vue.js and svelte. But Sciter.js is changing this and it doesn't has to support every browser's feature, as many javascript developers are using bundlers for code transformation and for supporting older browsers, and according to their experience, they should adapt with Sciter.js

Sciter.js is getting interesting and I started to think about the possibility to support Leaflet after some fixes including fixing styles. Likely, I'll try this in the near future.

Note: I'll check your last message about element.style.setProperty immediately.

jsprog commented 3 years ago

I can confirm that:

This issue got fixed under Linux
element.style.setProperty is working

Feel free to close this issue anytime and thank you for informing me about the wiki. I need to spend more time exploring this wiki and code samples.

c-smile commented 3 years ago

Some form of location will be there but in Sciter variant as url method:

const thisUrl = document.url("relpath");
cosnt relativeUrl = document.url("frames/relpath.htm");

Each framework has its own notion of routing.

c-smile / sciter-js-sdk

Fix element.insertBefore(node, refNode) to accept falsy second parmeter #8