Current implementation of DOM manipulation needs to be changed

KilianKilmister commented 4 years ago

The manipulation of the DOM using the Document.write method is not an optimal choice.

It is in fact acitvely blocked in chrome when used on a 2G connection. ([src1](https: //developers.google.com/web/updates/2016/08/removing-document-write), [src2](https: //stackoverflow.com/a/27531116/12945644))

i'd also suggest not passing object properties and formatting options together with the text.

in general, parsing data in string form decrypting that at the target requires unnescessary resources and can massively decrease code-maintainability and readability.
Data should be passed in proper JavaScript objects. If any translation from and/or to another dataformat is required (plain text is a data format in itself) this should be done as the first/last step with before receving/outputting the data plain data.
(if we count the Just-In-Time compilation from JS into machine language you can double the translations)
examlpe taken from /io.js

This function call in the test flow takes a Plain text string

this is the first translation from plain text to JS

 debugmsg('...Found (A): ' + test.testOutput[i])

The called function then has to translate another string

this function coud be skipped entirely as it doesn't add any nformation nescessary until it is outputted. All actual information dded here is a "debug"-flag.

 export function debugmsg (s) {
    /*  
     * this flag is provided with the scope and seems entirely unnescessary
     * to be checked, as a debug massage should not be called at all
     * unless you are trying to debug want to debug. 
    * If anythig it should throw a warnign
    */
   if (settings.debug) {
     io.addToOutputQueue('<p @@@@ class="debug">' + s + '</> p>')
     if (game.transcript) game.scriptAppend('D' + s) 
   }
 }

this function does even more processing processing

all the information this entire function actually adds is someArray.push(input) the rest is utterly unneccesary.

 io.addToOutputQueue = function (s) {
     // typechecking only nescessary because of the preprocessing into a string
   if (typeof s === 'string') {
       // now here the data is encoded into a key-value-pair `html: String`
       // this is implicit, as it otakes ALL strings and only outputs HTML.
       // the second key value pair is also implicit, as there are no other choices.
     io.outputQueue.push({ html: s.replace('@@@@', 'id="n' + io.nextid  '"'), action: 'output', id: io.nextid })
   } else {
       // the output-queue is an array,
       // so every stored value has a corresponding key by default
       // it also itroduces major riskks for desyncronisation, as `io.nextid` is just
       // some varioable in the global scope and not directly tied to 
       // the data flowing in and out.
     s.id = io.nextid
     io.outputQueue.push(s)
   }
   io.nextid++
     // imlicit. the postprocessor should simply call `outputQueue.push(data)` at the start
   io.outputFromQueue()
 }

postprocessor/translator

this last part should do exactly two things:

the postprocessor will take the object containing the data apart and serve it to where it's supposed to go. any function called from a postproccesor should not have to disect the data. but recieve it as neat arguments from here. if any translation has to be made, it will be passed to a translator, to be passed back to the postprocessor who sends it of to it's destination

the translator will recieves all instructions from the postprocessor and interpret it to translate it to another dataformat and send it off to the final destination.

io.outputFromQueue = function () {
 // implicit, this function should not be called unless there is something to output
 // throw an error, if anything.
if (io.outputQueue.length === 0) {
 io.enable
 return
}
 // the output function should not be called unless it should output something. 
 // move to seperate function and not at the beginning of the preprocesser.
if (io.outputSuspended) return
 // this part is simply baffeling to me. I don't see any reason to make this
 // perfectly good index utterly useless. all the data that is displayed in the main
 // output HAS to pass through `io.outputQueue`. It is the Ultimate reference point to
 // and from ANYTHING on the main portion of the screen. And with this reference
 // point, everything can be dynamically changed at will.
const data = io.outputQueue.shift()
 // `io.disable` is, `io.outputSuspend`, and the `wait()` function represent 3 versions to 
 // hard-pause program execution. They are also all interconnected AND exposed to the 
 // global scope of the package. A program should have 0-1 of those. 
 //And the only reason to have one trigger is through the enduser wanting to pause the program. If 
 // they are neccessary for program flow, then the flow is flawed. 
if (data.disable) io.disable()
 // major program operation. it should not be processed here.
 // instead the `data.action` property should be the actuall function call
 //   // `data.action = function()` and here the line `data.action()`
if (data.action === 'wait') {
 io.outputSuspended = true
 io.clickToContinueLink()
}
 // same here
if (data.action === 'delay') {
 setTimeout(io.outputFromQueue, data.delay * 1000)
}
 // only things that need outputting should use this function in the first place.
if (data.action === 'output') {
   // !!HERE!! this is the point where ALL the relevant
   // data of the of the input object should be processed.
   // There should be NO actual html before this point,
   // as this is the node between the code and the display.
 $('#output').append(data.html)
 io.outputFromQueue()
   // !!STOP!! and nothing should be done after this with the input. It has been sent to the renderer and that marks the point where the output function forgets the object ever existed.
}
 // I'm not sure what this is for, as all the references are commented out.
 // from what i can tell `effect` is somekind of encoding for rendering effects?
 // this should be handled like this. just include some callback function, like abouve.
if (data.action === 'effect') {
 data.effect(data)
}
window.scrollTo(0, document.getElementById('main').> scrollHeight)
}

ThePix commented 4 years ago

I would love to have a better way of doing these things, but there are reasons. Admittedly it could be that I do not know any better, and if you can find a better way that would be great. As usual this will sound negative; please do not take it that way.

"The manipulation of the DOM using the Document.write method is not an optimal choice."

I know, but as usual I am constrained by the fact that this has to be easy to use by authors. It has to be possible for one author to write a set of library files, and for a second author to be able to easily add those files to his game.

ETA: Using the bundler would be the way, I guess.

"i'd also suggest not passing object properties and formatting options together with the text."

Not sure I get this entirely. I get that processing a string several times is going to be slower, and I think the changes I did a couple of days ago may have improved this slightly?

Also, note that output can be held up occasionally. If you open page-eg.html and type in the command "reveal" you will see various effects and pauses that the output system has to support.

KilianKilmister commented 4 years ago

Being able to pass a function as the an object property to be executed in the final output step enables easy use of of plugins.

For example a passed function could send send the display text to google translate and the returned string would just pass along the line and be printed to screen.

Through this, we expose a controle point at the very end of the chain where anything can be injected without affecting the core of the engine.

I well take a look at it, but it’s not my main focus for right now

ThePix commented 3 years ago

I am going to close this as it has been hanging around so long.

I do not think there is an issue with document.write, as according to here it is only prohibited in fairly usual situations, including when the script is on another domain, so is not the case here. https://developers.google.com/web/updates/2016/08/removing-document-write

ThePix / QuestJS