Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rendering issues when used with ES6 / prototypes #27

Closed
arturi opened this issue May 31, 2016 · 10 comments
Closed

Rendering issues when used with ES6 / prototypes #27

arturi opened this issue May 31, 2016 · 10 comments

Comments

@arturi
Copy link

arturi commented May 31, 2016

Hey! Using yo-yo in a project with ES6 classes: we have Core and Plugins, and each plugin is inheriting from a Plugin class, which serves a boilerplate. Using super and extend keywords, which is relevant, because the issue goes away without those. Everything gets transpiled down to ES5 with Babel.

Sometimes yo-yo won’t render an element that was created in a constructor with this, like so:

import yo from 'yo-yo'

export class Smth extends Plugin {
  constructor (core, opts) {
    super(core, opts)
    this.strange = yo`<h1>this is strange 1</h1>`
  }

  render (state) {
    const bla = yo`<h2>this is strange 2</h2>`
    return yo`
      <div class="wow-this-works">
        <input type="text" value="hello">
        ${this.strange}
        ${bla}
      </div>
    `
  }

  install () {
    this.getTarget(this.opts.target, this, this.render)
  }
}

<h1>this is strange 1</h1> gets rendered while <h2>this is strange 2</h2> does not. If you move this.strange from the constructor to render function, it works. If you console.log the elements, they always show up in the console. Also, it works if you wrap yo call in a function, like this.strange = function () { return yo'<h1>this is strange 1</h1>' }

And I would think it’s totally this and class issues, except — wait for it — when you replace h2 with h1, it works. Different elements won’t render, but if two of the same are used — it works.

Here is a diff of two transpiled examples: https://www.diffchecker.com/ngpc5whl (the one on the left works, the one on the right does not), and are screenshots:

screen shot 2016-05-31 at 17 01 47

screen shot 2016-05-31 at 17 02 22

I thought maybe someone who wrote bel and knows internals will understand what’s going on here. Where should we even look? Sorry for complicated explanation and thanks in advance.

@shama
Copy link
Member

shama commented May 31, 2016

Hmm not sure at first glance. Would you mind throwing up an example in a repo that I could try?

@yoshuawuyts
Copy link
Member

yoshuawuyts commented May 31, 2016

Thanks for opening this issue! Ummmm, off the top of my head I have literally no idea what could be causing this - are you using yo-yoify at all? What about babel?

This probably definitely needs deeper research to figure out; would you mind setting up a quick repro so we can plough through it? Thanks!

edit: @shama jinx!

@arturi
Copy link
Author

arturi commented Jun 1, 2016

Thanks for willing to help! Sure, we have the repo on GitHub:

  1. git clone -b new-build https://github.com/transloadit/uppy/
  2. npm install
  3. npm run watch
  4. Open example/index.html in the browser, the Modal dialog with the example I mentioned should pop up.
  5. Edit src/plugins/Dummy.js and refresh the browser tab to see changes.

@arturi arturi changed the title Rendering issues when used with ES6 Rendering issues when used with ES6 / prototypes Jun 1, 2016
@shama
Copy link
Member

shama commented Jun 1, 2016

Thanks for the repo example! Using that, I built a reduced test to try and pinpoint the issue. I replaced src/index.js with the following:

import yo from 'yo-yo'

class Plugin {
  constructor (core, opts) {
    this.core = core
    this.opts = opts
  }
}

class Dummy extends Plugin {
  constructor () {
    super({
      state: { what: 'strange' }
    }, {})
    this.strange = yo`<h1>this is strange 1</h1>`
  }

  render () {
    const bla = yo`<h1>this is ${this.core.state.what} 2</h1>`
    return yo`
      <div class="wow-this-works">
        <input type="text" value="hello">
        ${this.strange}
        ${bla}
      </div>
    `
  }
}

var dummy = new Dummy()
var root = dummy.render()

document.body.appendChild(root)

and it renders fine for me:
screen shot 2016-06-01 at 12 26 37 pm


I'm not sure what the issue is but I don't think this is related to bel, babel or the <h1> tag. I noticed when logging in the render() method of src/plugins/Dummy.js, it gets called a bunch of times.

My guess at the problem is you're running into a timing issue. Where one call to render() is winning against another call to render() when the state might not be in the place you would expect. I was having trouble debugging as it appears debugger statements get removed in the build process (and I can't read babel generated code) so unfortunately don't know the origin of the issue.

A way to combat these types of timing issues is to use a data down, actions up architecture. Where the state is basically an object that lives at the top and is passed into each plugin upon render. The plugin manipulates the data as need to build the desired HTML element and returns the element. Any time an event or async action needs to occur, an action (could be an event emitter, redux thing or just a callback passed down with the data) is called. The action manipulates the state and then sends the data down again to render() to complete the cycle.

Then just avoid any types of observables triggering a render() call or saving/manipulating a cached this.el on the plugins themselves.

@arturi
Copy link
Author

arturi commented Jun 1, 2016

Hey! Thanks so much for digging into this.

The problem does not occur if I mount an element directly to the DOM, like in your example. It occurs when the render function is passed to the other plugin and then called there inside another render with yo-yo. See here: https://github.com/transloadit/uppy/blob/master/src/plugins/Modal.js#L207.

Our system works like this: Core runs all the plugins and keeps the state, as you are describing. It also has actions that modify said state. Each plugin can be installed directly to the DOM element or to other plugin, which is the case when we have a Modal dialog and we want the DragDrop plugin (or Dummy in this example) to be installed to that Modal dialog, and not directly to the DOM.

The reason render is called multiple times is because each time the state is updated, using the setState method from the Core, updateAll is called: https://github.com/transloadit/uppy/blob/master/src/core/Core.js#L45. That iterates over all the installed plugins and calls update on them, which in turn calls render. So each time anything in the app changes, all of the plugins’ render methods are called.

Have you witnessed the h1 vs h2 issue though? I would agree it’s a race condition of some sort as you are describing, if that wasn’t happening.

@shama
Copy link
Member

shama commented Jun 1, 2016

When I ran the build, the tag didn't matter. It started as a <h1>, I changed either to an <h2> and then either to a <div>. Same result, this is strange 1 doesn't show for me in all cases.

@arturi
Copy link
Author

arturi commented Jun 1, 2016

Sorry, could you pull now, this makes it “work”: transloadit/uppy@9a2d81f

Same tags show up, different don’t.

@shama
Copy link
Member

shama commented Jun 2, 2016

Ah yeah, now it happens for me. It appears random though. I tried with other tags, some work and some don't. It's highly unlikely it's related to the specific tags though. It renders just fine using Babel and yo-yo on their own within your build system, just not when integrated into the Plugin system.

My guess is still timing issues with the architecture. Upon init, it renders on Dummy four times. It should only need to render once. I bet when Modal is rendering, the this.strange property is undefined as one of those render()s are called. It's hard to know which render led to the element that ended up in the DOM. There's a bit of context juggling happening as you're passing about the targets too. That could be the cause as well. I find it really hard to debug Babel generated code, sorry.


The architecture I am describing is different. Data down, actions up looks like a tree. You pass data to a parent. The parent uses that data to create children and continues passing it further and further down creating a tree. Child elements do not talk to other child elements. They send an action up to the parent to either pass further up or handle itself. They talk through actions manipulating the data and rendering it back down.

A core plugin manager is not needed, IMO. As plugins, each element you create has a peer dependency to that core plugin manager. Modifying the API of that framework means all your elements break until you update them. Also those elements will be unusable to me and the larger community.

yo-yo is really designed for standalone, data down, actions up elements (like a yo-yo). For example, if Modal uses Dummy, then it should import and create Dummy instances in the constructor and call dummy.render(state) when Modal.render() is called.

Or even more simply:

import yo from 'yo-yo'
import dummyElement from './dummy.js'

export default function modalElement (state) {
  return yo`<div class="modal">
    ${dummyElement(state)}
  </div>`
}

Or make the modal a more generic element:

import yo from 'yo-yo'
export default function modalElement (contents) {
  return yo`<div class="modal">
    ${contents}
  </div>`
}
import yo from 'yo-yo'
import modalElement from './modal.js'
import dummyElement from './dummy.js'

let element = modalElement(dummyElement())

I'm not saying any path is right or wrong but this is a path of least resistance with yo-yo. :)

@arturi
Copy link
Author

arturi commented Jun 2, 2016

It appears random though. I tried with other tags, some work and some don't.

Not so random, I too tried with different tags, it’s not the tag that matters, but whether or not I use two of the same. It renders all the time with address + address, h1 + h1, span + span etc. Fails with span + div, h1 + h2, a + button.

It renders just fine using Babel and yo-yo on their own within your build system, just not when integrated into the Plugin system.

That was my experience too.

I’ve managed to solve the issue by passing just the render function and not the pre-rendered element to the target, and binding this to a few methods in here transloadit/uppy@5eadd13#diff-24345cdf8d762d6253b9e81d7f2e8238L37 and here transloadit/uppy@5eadd13#diff-ad9cd752323f4e279fdd37d35a0532c7L20. It works, and the extra-network-request issue is also solved by caching the preview img element in the file object, as suggested by @yoshuawuyts. Magic: http://uppy.io/examples/modal/index.html.

The “two same or different elements” still remains a mystery to me, but I do agree it’s probably the timing / rendering issue. Maybe at some point it was quicker to render two of the same elements? In console it always showed the element though, never undefined.


Child elements do not talk to other child elements

Thanks for all the examples and explanations again. I do understand what you are saying and will re-read a few times to make sure it sinks in, really appreciated.

What you are describing is the general usage pattern for React, Virtual Dom and Yo-Yo, as far as I can tell by exploring a bunch of applications, like WebTorrent Desktop, Chatwizard, your csv-viewer/fs-explorer and others.

For Uppy though, we planned for the end user to be able to use it as a ready app, without having to combine components themselves, like so:

import Uppy from 'Core.js'
import Dummy from 'Dummy.js'
import Modal from 'Modal.js'
import DragDrop from 'DragDrop.js'
import GoogleDrive from 'GoogleDrive.js'
import ProgressBar from 'ProgressBar.js'

const uppy = new Uppy({debug: true, autoProceed: false})
  .use(Modal, {trigger: '#uppyModalOpener'})
  .use(ProgressBar, {target: 'body'})
  .use(DragDrop, {target: Modal})
  .use(GoogleDrive, {target: Modal, host: 'http://server.uppy.io'})
  .use(Dummy, {target: Modal})
  .run()

Or even a pre-bundled version with a script tag:

<button id="uppyModalOpener">Open Modal</button>
<link href="../dist/uppy.min.css" rel="stylesheet">
<script src="../dist/uppy.min.js"></script>
<script src="../dist/locales/ru_RU.min.js"></script>
<script>
  var uppy = new Uppy.Core({locales: Uppy.locales.ru_RU, debug: true})
    .use(Uppy.plugins.Modal, {trigger: '#uppyModalOpener'})
    .use(Uppy.plugins.Dummy, {target: Uppy.plugins.Modal})
    .run();
</script>

In here all of these components share the same state, and all of them are re-rendered when any of that state changes, but they do not always have the parent --> child relationship. Because we thought maybe you want to install Modal to body, DragDrop & GoogleDrive to Modal, and then ProgressBar to the body again. Or more simply: the user can choose to either use the Modal and install everything to it, or just install everything to some dom element.

It is more complicated and error-prone, as we have seen, but I am not sure if we should make the user re-create the app himself, and just provide the components. Maybe it makes sense to supply the bundled version for those who want it simple, and then doing the component-way you are describing for those who want more control.

The other issue is that our plugins are not just views, they have logic like Google Drive authentication, and since it’s a plugin, this logic can’t be bundled into the core/root component, which is usually the case with this types of architecture. So plugins need to extend actions/dispatcher.

@arturi
Copy link
Author

arturi commented Jun 22, 2016

Thanks again for all your help, closing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants