choojs / nanocomponent-adapters

🔌 - Convert a nanocomponent to a component for your favourite API or library (web components, (p)react, angular)
MIT License
96 stars 17 forks source link

toReact not working as expected #16

Closed jongacnik closed 6 years ago

jongacnik commented 7 years ago

The current toReact adapter from https://github.com/choojs/nanocomponent-adapters/pull/15 does not work as expected. It always returns a new component, and therefore always a new nanocomponent.

If you convert the adapter to use es6 classes, it will work as expected:

function toReact (Component, react) {
  assert.equal(typeof Component, 'function', 'nanocomponent-adapters/react: component should be type function')
  assert.equal(typeof react, 'object', 'nanocomponent-adapters/react: react should be type object')

  class Clx extends react.Component {
    constructor () {
      super()
      this.comp = new Component()
      this.node = null
      this.setRef = this.setRef.bind(this)
    }

    componentDidMount () {
      if (!this.comp.element) {
        var el = this.comp.render(this.props)
        this.node.appendChild(el)
      }
    }

    setRef (_node) {
      this.node = _node
    }

    componentWillReceiveProps (props) {
      if (this.comp.element) this.comp.render(props)
    }

    shouldComponentUpdate () {
      return false
    }

    render (props) {
      return react.createElement('div', { ref: this.setRef })
    }
  }

  return Clx
}

Here's a little demo. If you increment both components, and then trigger a full app re-render, the prototype based component will remount and es6 based will remain:

demo: https://preact-adapter-test.glitch.me code: https://glitch.com/edit/#!/preact-adapter-test?path=public/client.js:1:1

If we're open to using es6 classes I can open a PR request, if not, I was getting a bit stumped by the prototypes so maybe someone can help take a look!

goto-bus-stop commented 6 years ago

oh dang i missed this. i don't know if ES classes are okay with everyone but i fixed the mistake that caused this in #17

jongacnik commented 6 years ago

rad! no need to swap to es classes if this fixes it. I was just having trouble pinpointing where the mistake was. lgtm.