C’è qualcosa di sbagliato o difficile nell’applicare una patch al virtual-dom per risolvere questo problema?
Un PR è benvenuto, ci abbiamo provato ma era semplicemente troppo difficile
2 Mi Piace
Molto interessante! Passare a un fork di vdom non è l’ideale, ma potremmo essere in grado di prenderlo in considerazione a condizione che possiamo dimostrare che non ci sono regressioni (e spostarlo nella nostra organizzazione GitHub).
Sei stato in grado di eseguire la suite di test vdom per confermare che non ci sono regressioni? Quanto sarebbe facile aggiungere un nuovo test vdom per questo comportamento di prepend?
2 Mi Piace
Affronterò prima la seconda domanda. Questo comportamento di preposizione è essenzialmente lo stesso, elimina solo un po’ di lavoro extra. Cercherò di illustrare cosa fa vdom originariamente e dopo la patch.
Supponiamo che ci siano 20 post e 10 vengano caricati in cima dopo lo scorrimento.
Originariamente vdom esegue questa sequenza di trasformazioni:
[20 vecchi] → [20 vecchi, 10 nuovi] // 10 nuovi elementi vengono aggiunti alla fine
[20 vecchi, 10 nuovi] → // tutti i 30 elementi vengono rimossi
→ [10 nuovi, 20 vecchi] // tutti i 30 elementi vengono inseriti al loro posto
Dopo la patch la sequenza diventa:
[20 vecchi] → [20 vecchi, 10 nuovi] // 10 nuovi elementi vengono aggiunti alla fine
[20 vecchi, 10 nuovi] → [20 vecchi] // 10 nuovi elementi vengono rimossi dalla fine
[20 vecchi] → [10 nuovi, 20 vecchi] // 10 nuovi elementi vengono anteposti
Come puoi vedere c’è ancora lavoro extra: in linea di principio puoi anteporre quegli elementi subito, ma in questo modo ho potuto aggiungere del codice in un posto e non preoccuparmi di riscrivere ciò che c’era già.
Ognuna delle trasformazioni diventa un’operazione sul DOM, quindi i video di YouTube iniziano a essere riprodotti perché originariamente gli elementi vecchi vengono reinseriti (il che li fa renderizzare di nuovo), e dopo la patch rimangono al loro posto.
2 Mi Piace
Ecco un estratto del file package.json di virtual-dom
"scripts": {
"test": "node ./test/index.js | tap-spec",
"dot": "node ./test/index.js | tap-dot",
"start": "node ./index.js",
"cover": "istanbul cover --report html --print detail ./test/index.js",
"view-cover": "istanbul report html & opn ./coverage/index.html",
"browser": "run-browser test/index.js",
"phantom": "run-browser test/index.js -b | tap-spec",
"dist": "browserify --standalone virtual-dom index.js > dist/virtual-dom.js",
"travis-test": "npm run phantom && npm run cover && istanbul report lcov && ((cat coverage/lcov.info | coveralls) || exit 0)",
"release": "npm run release-patch",
"release-patch": "git checkout master && npm version patch && git push origin master --tags && npm publish",
"release-minor": "git checkout master && npm version minor && git push origin master --tags && npm publish",
"release-major": "git checkout master && npm version major && git push origin master --tags && npm publish"
},
Di questi, ‘test’, ‘dot’, ‘cover’, ‘view-cover’, ‘browser’, ‘phantom’, ‘travis-test’ sembrano rilevanti per il testing.
‘browser’, ‘phantom’, ‘travis-test’ danno un errore di parsing a causa di costrutti javascript più recenti nel mio codice. Gli altri passano. Se cambio questo codice
var prepend = simulate.every(item => item && item.key)
prepend &= aChildren.every((item, i) => item.key === bChildren[i + shift].key)
in questo codice
var prepend = true
for (var i = 0; prepend && i < simulate.length; i++) {
prepend = simulate[i] && simulate[i].key
}
for (var i = 0; prepend && i < aChildren.length; i++) {
prepend = aChildren[i].key === bChildren[i + shift].key
}
allora tutti passano. Posso inviare questa modifica per mantenere javascript coerentemente vecchio se si desidera soddisfare tutti questi script.
2 Mi Piace
per favore fallo - manteniamo funzionante la loro suite di test esistente
Ho appena forkato virtual-dom in GitHub - discourse/virtual-dom: A Virtual DOM and diffing algorithm - puoi per favore fare una PR contro quel repository?
3 Mi Piace
3 Mi Piace
Devo anche notare che la mia modifica a virtual-dom è molto limitata. Si rivolge specificamente ai prepend e si basa sull’algoritmo originale in tutti gli altri casi. E quell’algoritmo originale è ancora imperfetto se si guarda al problema in generale (non è difficile trovare esempi in cui toccherebbe elementi vecchi inutilmente). D’altra parte, gestisce append, rimozioni, inserimenti singoli senza problemi. E con questo è necessario diventare molto fantasiosi con lo stream post per uscire. Quindi, in pratica, risolvere il problema in generale potrebbe essere un po’ eccessivo, anche se ovviamente si può dormire sonni tranquilli quando lo si fa.
Ho inviato alcuni nuovi test. Hai intenzione di rivedere la PR a breve?
1 Mi Piace
Grazie Aleksey, cercherò di farlo revisionare entro la prossima settimana.
4 Mi Piace
Ho configurato il fork di virtualdom e posso confermare che risolve il problema dell’iframe nei test manuali.
@Aleksey_Bogdanov Mi chiedo se puoi aiutarmi con qualcosa, però. Ho provato ad aggiungere un test all’interno di Discourse. Essenzialmente:
-
Renderizza
<span>ElementOne</span><span>ElementTwo</span> -
Rimuovi ElementTwo e anteponi
PrependedElement -
Verifica che il risultato sia
<span>PrependedElement</span><span>ElementOne</span> -
Verifica che lo span originale di ElementOne sia uguale allo span finale di ElementOne (cioè, verifica che non sia stato renderizzato nuovamente)
Sfortunatamente, il controllo al punto (4) fallisce, il che significa che ElementOne è stato renderizzato nuovamente. Hai qualche idea sul perché la nuova logica non funzioni in questo caso? 
Ho caricato la nuova configurazione del fork e il test fallito in questo PR esistente.
3 Mi Piace
Sì, ne ho scritto in precedenza.
Non mi aspetterei che questo funzioni in generale (e che il test, come lo descrivi, passi). Originariamente, funziona per aggiunte, rimozioni, inserimenti singoli (forse una combinazione di essi). Con la mia modifica funziona anche per pure prepends, che dovrebbero coprire il bug di YouTube. Il tuo test ha ‘rimozione + prepend’. Questo non è coperto dalla mia modifica.
Una soluzione più robusta richiederebbe una riscrittura significativa di ‘reorder’ e ‘diffChildren’ in virtual-dom almeno. Cosa che posso provare, ma questo pone la domanda: hai intenzione di mantenere il tuo fork di virtual-dom a lungo termine? Se stiamo cercando di essere robusti, potrebbe essere un’idea migliore e richiedere meno tempo e sforzo provare a passare a una libreria analoga attivamente mantenuta. La mia ipotesi è che altre librerie abbiano già risolto questo problema a quest’ora.
2 Mi Piace
Capisco, ha senso. Tuttavia, anche se lo rendo un ‘prepend puro’, ottengo lo stesso errore. Impostando un breakpoint nella funzione reorder() pertinente, si vede che si blocca al controllo bFree.length === bChildren.length:
Ho caricato il test aggiornato sul branch.
Questa è decisamente solo una soluzione a breve termine. Abbiamo già iniziato a sostituire il nostro uso di vdom con il rendering Ember/Glimmer. Il nostro obiettivo è sostituire l’implementazione dello stream di post entro i prossimi 12 mesi. Glimmer gestisce questo tipo di ‘prepend’ correttamente.
Quindi, sono totalmente felice di unire una modifica ‘incompleta, ma migliore di prima’. Ma, se non è troppo lavoro, sarebbe bello capire perché questo test non funziona
3 Mi Piace
Assolutamente. Non sono ancora sicuro, ma ci darò un’occhiata appena potrò.
3 Mi Piace
Aggiornamento.
Ho aggiunto le chiavi affinché il test passi. Le chiavi indicano a virtual-dom quali elementi corrispondono a quali. Senza di esse, virtual-dom presume semplicemente che gli elementi seguano lo stesso ordine di prima e non riesce a capire che si tratta di prepends.
Poi ho cambiato un singolo prepend in più prepend perché virtual-dom copriva già inserimenti singoli (e i prepend singoli sono un caso speciale), quindi non dovrebbero fare differenza.
Le modifiche sopra sono quelle che ho caricato finora.
Ma ora i prepend multipli non fanno differenza, il che è sorprendente. La mia teoria iniziale su cosa c’è di sbagliato e, per estensione, sul perché la mia patch funzioni potrebbe essere errata. Quindi sto ancora cercando di capirlo.
Noto, David, che hai debuggato il test nel browser. È difficile da configurare? Sto avendo difficoltà a configurare un debugger per i test e apprezzerei se potessi darmi qualche dritta.
3 Mi Piace
[quote=“Aleksey Bogdanov, post:59, topic:57692, username:Aleksey_Bogdanov”]
Ho aggiunto le chiavi per superare il test. Le chiavi suggeriscono quali elementi del virtual-dom corrispondono a quali
[/quote]Aha, capisco, grazie!
Hai una configurazione di sviluppo Discourse funzionante? Se sì, avviala, visita /tests nel browser, quindi usa l’interfaccia di filtro in alto per cercare “avoids rerendering on prepend”. Puoi quindi utilizzare gli strumenti di sviluppo del browser per il debug. (ad esempio, vai alla scheda sorgenti, ctrl+p per aprire il file, cerca vdom/diff e imposta un breakpoint)
3 Mi Piace
Grazie, ci proverò.
3 Mi Piace
Quindi, questo codice
assert.strictEqual(elementOneBefore, elementOneAfter);
non fa differenza perché elementOneBefore mantiene la sua identità in tutti questi cambiamenti in entrambi i casi.
Ecco una piccola illustrazione, se desideri vederla tu stesso:
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title>Youtube Bug Demo</title>
</head>
<body>
<script>
let iframe = document.createElement("iframe");
iframe.width = 690;
iframe.height = 388;
iframe.src = "https://www.youtube.com/embed/Xc5rB-0ZBcI";
iframe.title = "Strange S.T.A.L.K.E.R. car glitch";
document.body.appendChild(iframe);
let button = document.createElement("button");
button.type = "button";
button.innerHTML = "Reinsert";
button.onclick = function(){
let iframeStart = document.querySelector("iframe");
document.body.removeChild(iframeStart);
document.body.insertBefore(iframeStart, button);
let iframeEnd = document.querySelector("iframe");
alert(iframeStart === iframeEnd);
};
document.body.appendChild(button);
</script>
</body>
</html>
Ciò che fa la differenza sono quelle chiamate removeChild, insertBefore, quindi ho aggiunto un controllo per le mutazioni del DOM. Ora il test fallisce per la versione precedente e passa per quella corrente, quindi spero che questo sia sufficiente.
6 Mi Piace
Ottimo, grazie mille per il tuo lavoro qui @Aleksey_Bogdanov. Ho appena unito la PR: sarà attiva su Meta entro la prossima ora.
6 Mi Piace