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

Enable embedding self-contained apps in views #241

Closed
wants to merge 1 commit into from
Closed

Enable embedding self-contained apps in views #241

wants to merge 1 commit into from

Conversation

zaceno
Copy link
Contributor

@zaceno zaceno commented Jun 21, 2017

This allows an app to declare that it wants to be rendered at
a specific position among siblings within it's root, by
declaring the element at that position to the renderer.

The primary motivation for this, is that it is possible to embed
self contained apps inside views, by rendering a "dummy" virtual
node with an oncreate handler that instantiates the embedded app.

Without this change, however, apps embedded like this will be
rendered at the end of the list of siblings, because the
renderer does not know any better.

With this change, the instantiation of the embedded app can
inform the renderer to render the app in the position of the dummy
vnode

This allows an app to declare that it wants to be rendered at
a specific position among siblings within it's root, by
declaring the element at that position to the renderer.

The primary motivation for this, is that it is possible to embed
self contained apps inside views, by rendering a "dummy" virtual
node with an oncreate handler that instantiates the embedded app.

Without this change, however, apps embedded like this will be
rendered at the *end* of the list of siblings, because the
renderer does not know any better.

*With* this change, the instantiation of the embedded app can
inform the renderer to render the app in the position of the dummy
vnode
@codecov
Copy link

codecov bot commented Jun 21, 2017

Codecov Report

Merging #241 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #241   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         166    167    +1     
  Branches       45     45           
=====================================
+ Hits          166    167    +1
Impacted Files Coverage Δ
src/app.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6606a36...f0a30a9. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 21, 2017

Codecov Report

Merging #241 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #241   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         166    167    +1     
  Branches       45     45           
=====================================
+ Hits          166    167    +1
Impacted Files Coverage Δ
src/app.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6606a36...f0a30a9. Read the comment docs.

@zaceno
Copy link
Contributor Author

zaceno commented Jun 21, 2017

...yes this basically means enabling "components with local state" or "stateful components". But I think perhaps "embedded apps" is less controversial around here ;)

Nobody worry though! This teensy patch to hyperapp does not build in support for stateful components into hyperapp -- they must come from a user-land function. But that function can't work properly without this patch.

The userland function for "embedding an app" (= creating a stateful component) might look something like this:

function embed (opts) {
  return {
    tag: 'div',
    children: [],
    data: {
      key: opts.key,
      oncreate: el => setTimeout(_ => {
        opts.root = el.parentNode
        opts.element = el
        app(opts)
        el.parentNode.removeChild(el)
      })
    }
  }
}

And you might use it like this:

const MyStatefulInputComponent = (props, children) => embed({
  state: props.value,
  actions: {
    set: (_, __, newValue) => {
      props.notifyParent(props.fieldName, newValue)
      return newValue
    }
  },
  view: (val, {set}) => (<input type="text" value={val} oninput={ev => set(ev.currentTarget.value)} />)
})

app({
  state: ...
  actions: ...
  view: (state, actions) => (<form>
    {state.fields.map(({name, value}) => (
    <MyStatefulInputComponent
      fieldName={name}
      value={value}
      notifyParent={actions.notifyMe}
    />
    ))}
  </form>)
})

@zaceno zaceno requested a review from jorgebucaran June 21, 2017 22:47
@zaceno zaceno mentioned this pull request Jun 21, 2017
@jorgebucaran
Copy link
Owner

jorgebucaran commented Jun 22, 2017

@zaceno This would open the door to decentralized state, which is not a pattern I'd like to promote in HyperApp. This would be an interesting path and architecture to explore on a different project, perhaps even based on picodom.

@zaceno
Copy link
Contributor Author

zaceno commented Jun 22, 2017

@jbucaran Sure, and in fact I'm already using this approach (with my homemade state management + picodom) in my own projects. So I'm not heavily invested in this PR personally.

Still: I think it's wise to separate the ideas of what people can do, and what you would like (or not like) them to do. It's a one-line-diff after all.

This would open the door to decentralized state

The door is already open, in the sense that you can call app('....') multiple times in a page. And that is as it should be, I think. If you want to close that door, it should be more clear from the design and/or docs that you're only meant to use app(...) once per browser tab.

But I personally like the fact that there can be multiple apps. The only thing is that it is not very easy to set up interactions between them because there's no interface from the outside. Something equivalent to Elm's ports would be nice.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Jun 22, 2017

The door is already open, in the sense that you can call app('....') multiple times in a page.

Yes, but they can't communicate with each other unless you use a global emitter or another message exchange device. So, it's not an open door; it's more like a rope bridge.

I think it's wise to separate the ideas of what people can do, and what you would like (or not like) them to do.

You are right, which is why I've tried not to take sides with bundlers, template literal fanatics, JSX objectors, etc. But I have to be careful not to break the original TEA promise, because then what would be the ideological benefit of using HyperApp over Mithril or Preact?

I think there's plenty of room for another tiny framework that is not constrained to React's API (like Preact) and can still coexist with Preact, but that is not HyperApp.

I am putting all my money (figuratively) on #238. 😄

Something equivalent to Elm's ports would be nice.

IIRC ports exist to wrap 3rd party libraries and because Elm is strongly typed, not as a bridge to communicate with different apps all of which are running the entire Elm runtime. I could be wrong.

EDIT: Grammar.

@zaceno
Copy link
Contributor Author

zaceno commented Jun 22, 2017

... more like a rope bridge

😆 I like that analogy!

I have to be careful not to break the original TEA promise

I get that. Just to be clear though: this PR would not break the TEA promise in any way. I'm not suggesting that you incorporate the embed example above into hyperapp. The diff is still just a one-liner.

Yet it does open the door for others to break TEA if they so choose.

IIRC ports exist to wrap 3rd party libraries

Good point. I haven't used them really, so I just made an assumption what they were for.

@zaceno
Copy link
Contributor Author

zaceno commented Jun 22, 2017

Actually, I just realized it's possible to use the embed patttern above even without this PR.

This should work:

function embed (opts) {
  return {
    tag: 'div',
    children: [],
    data: {
      key: opts.key,
      oncreate: el => setTimeout(_ => {
        const fakeRoot = document.createElement('div')
        opts.root = fakeRoot
        app(opts)
        el.parentNode.replaceChild(fakeRoot.childNodes[0], el)
      })
    }
  }
}

... hence I am closing this PR

@zaceno zaceno closed this Jun 22, 2017
@zaceno zaceno deleted the allow-embedded-apps branch June 22, 2017 08:08
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

Successfully merging this pull request may close these issues.

None yet

2 participants