choojs / choo

:steam_locomotive::train: - sturdy 4kb frontend framework
https://choo.io/
MIT License
6.78k stars 595 forks source link

Embedding tweets in choo #290

Closed notenoughneon closed 7 years ago

notenoughneon commented 8 years ago

Sorry if this is not actually a bug, but I don't know how to proceed with this issue and would appreciate any help.

I made a chat app using choo, and added support for oembedding various types of links (youtube, soundcloud, etc). I add the oembed html content to a div's innerHTML, which works fine for youtube and soundcloud, but twitter is giving me trouble. Twitter oembeds include a script that loads asynchronously and modifies the dom to apply styling.

Unstyled: unstyled

Styled: styled

I tried using twttr.widgets.load() and twttr.widgets.createTweet() as documented on their website: https://dev.twitter.com/web/javascript/initialization

The problem I can't get around is that every time choo updates the view, it re-renders the tweet, causing flickering and changing the scroll position. It will also interrupt embedded media players. It seems like the dom diffing is not working properly. Stuff I tried to fix this:

I set up a demo here: https://nekocafe-tgodwqhzlf.now.sh/ And the code is here: https://github.com/notenoughneon/nekocafe/commit/20b4bab2fe5c52b1800d626044479c4ae7686289

timwis commented 8 years ago

Hi @notenoughneon - we've seen this issue before with things like leaflet and d3 and other libraries that alter the DOM themselves. Try cache-element out for that -- it's fresh off the grill, so do let us know how it works out!

notenoughneon commented 7 years ago

Thanks for the suggestion! I will let you know how it goes.

notenoughneon commented 7 years ago

I tried wrapping the element with cache, but something is not working. The compare function is always receiving 0 as the prev value, so only the first tweet gets rendered.

https://github.com/notenoughneon/nekocafe/commit/ad73cfef2c528281e5e35cf327bb42e706136060

yoshuawuyts commented 7 years ago

oh yeah, I think what's going on is that you're not passing the prev value into the element. Each view is passed (state, prev, send) as arguments. This was done so that cache-element doesn't accidentally hold references to old data and cause memory leaks - all needs to be passed in explicitely

yoshuawuyts commented 7 years ago

edit: hope that makes sense :sparkles:

notenoughneon commented 7 years ago

I'm not sure where to get prev. I thought that was the job of cache-element?

What I am trying to do:

const embedTweet = (tweet) => {
    const div = html`<div>${tweet}</div>`
    twttr.widgets.load(div)    // *** slow and asynchronous ***
    return div
}

const renderWidget = someCacheFunction(embedTweet)

// the following calls to renderWidget:
renderWidget('tweet a') // creates new element
renderWidget('tweet b') // creates new element
renderWidget('tweet a') // returns cached 'a'
renderWidget('tweet b') // creates cached 'b'
renderWidget('tweet c') // creates new element
yoshuawuyts commented 7 years ago

Oh yeah so prev is returned by the view: the signature in choo 3.x is (state, prev, send)

On Tue, Nov 22, 2016 at 7:58 PM Emma Kuo notifications@github.com wrote:

I'm not sure where to get prev. I thought that was the job of cache-element?

What I am trying to do:

const embedTweet = (tweet) => { const div = html<div>${tweet}</div> twttr.widgets.load(div) // * slow and asynchronous * return div } const renderWidget = someCacheFunction(embedTweet) // the following calls to renderWidget:renderWidget('tweet a') // creates new elementrenderWidget('tweet b') // creates new elementrenderWidget('tweet a') // returns cached 'a'renderWidget('tweet b') // creates cached 'b'renderWidget('tweet c') // creates new element

β€” You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/yoshuawuyts/choo/issues/290#issuecomment-262332124, or mute the thread https://github.com/notifications/unsubscribe-auth/ACWlemrNsCPNCrtPYsvpv0rawtxK92OLks5rAztlgaJpZM4KeN0A .

notenoughneon commented 7 years ago

I updated to choo 4, will this still work?

yoshuawuyts commented 7 years ago

yeah it should!

On Tue, Nov 22, 2016 at 8:04 PM Emma Kuo notifications@github.com wrote:

I updated to choo 4, will this still work?

β€” You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/yoshuawuyts/choo/issues/290#issuecomment-262333869, or mute the thread https://github.com/notifications/unsubscribe-auth/ACWlen-DdsFumzotcHmU7bP7W2jBdCgkks5rAzzWgaJpZM4KeN0A .

notenoughneon commented 7 years ago

I tried passing the prev values down from the root view, but it is still only rendering the first tweet.

https://github.com/notenoughneon/nekocafe/commit/808ad0648b1d8791bd8e3507a9474df7f5f77f81

yoshuawuyts commented 7 years ago

ah yeah I think I know what's up: cachedEmbed is a stateful object which is being reused multiple times; if you wanna cache multiple elements you should probably create multiple instances of the element in a constructor. I think your use case makes a lot of sense, and we should probably cater better for this. Sorry the docs on this are a bit rough πŸ˜”

On Tue, Nov 22, 2016 at 8:56 PM Emma Kuo notifications@github.com wrote:

I tried passing the prev values down from the root view, but it is still only rendering the first tweet.

notenoughneon/nekocafe@808ad06 https://github.com/notenoughneon/nekocafe/commit/808ad0648b1d8791bd8e3507a9474df7f5f77f81

β€” You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/yoshuawuyts/choo/issues/290#issuecomment-262348111, or mute the thread https://github.com/notifications/unsubscribe-auth/ACWleilEvtgp3kvtw-0nvhVFF9EPUSzpks5rA0jlgaJpZM4KeN0A .

notenoughneon commented 7 years ago

After playing around with it some more, I think I found a solution. I tried this earlier but isSameNode only seems to work in choo 4.

const cache = {}

function renderOnce(render) {
    return arg => {
        if (cache[arg]) {
            const proxy = html`<div></div>`
            proxy.isSameNode = () => true
            return proxy
        }
        cache[arg] = true
        return render(arg)
    }
}

const embedRow = renderOnce((snippet) => {
    const embed = html`<div></div>`;
    embed.innerHTML = snippet;
    twttr.widgets.load(embed);
    return html`
        <div class="row">
            <div class="col-xs-12 col-md-8">
                ${embed}
            </div>
        </div>
    `;
});
leeoniya commented 7 years ago

no disrespect to choo or authors, but seems like a lot of contortions to get this done :/

EDIT: I guess it's a consequence of using morphdom (which advertises "no virtual dom!"). But in reality the fastest dom-diffing libs all use a virtual dom [3].

[1] https://rawgit.com/leeoniya/domvm/2.x-dev/demos/embed-twitter.html [2] https://github.com/leeoniya/domvm/blob/2.x-dev/demos/embed-twitter.html [3] https://rawgit.com/krausest/js-framework-benchmark/master/webdriver-ts/table.html

yoshuawuyts commented 7 years ago

Yay, glad you got it to work! - from talk on IRC folks are only now starting to use caching - the whole thing is still a bit rough; real glad you managed to get it to work - hopefully we can soften up the docs a little as we progress (:

On Wed, Nov 23, 2016 at 8:32 PM Leon Sorokin notifications@github.com wrote:

no disrespect to choo or authors, but seems like a lot of contortions to get this done :/

[1] https://github.com/leeoniya/domvm/blob/2.x-dev/demos/embed-twitter.html

β€” You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/yoshuawuyts/choo/issues/290#issuecomment-262610400, or mute the thread https://github.com/notifications/unsubscribe-auth/ACWlerYu5jQagQOR4iKaB54WJ26Wp9kbks5rBJTjgaJpZM4KeN0A .

notenoughneon commented 7 years ago

I found a couple problems with my last solution.

  1. Multiple instances of the same tweet can't coexist, only one will display
  2. Hiding and reshowing a tweet doesn't work

Here is my next attempt, which fixes those problems at the expense of re-invoking createTweet() on every refresh.

const embedRow = (embed) => {
    const inner = html`<div></div>`;
    switch (embed.type) {
        case 'html':
            inner.innerHTML = embed.html;
            break;
        case 'tweet':
            twttr.widgets.createTweet(embed.id, inner);
            break;
    }
    const elt = html`
        <div class="row">
            <div class="col-xs-12 col-md-8">
                ${inner}
            </div>
        </div>
    `;
    elt.isSameNode = () => true;
    return elt;
};
yoshuawuyts commented 7 years ago

https://www.npmjs.com/package/nanocomponent exists now for this purpose - thanks!