Lazy components

[frenzzy]

• Lazy execute components at runtime, probably it may save computation in some cases
• It will be possible to easily extend component with connect proposal, ref Add connected components #604
• It will be possible to compile JSX right into virtual node objects avoiding h function with components support, ref Compile JSX right into virtual node objects avoiding h function #564

[codecov]

Codecov Report

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

@@          Coverage Diff          @@
##           master   #606   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines         139    140    +1     
  Branches       42     42           
=====================================
+ Hits          139    140    +1

Impacted Files	Coverage Δ
src/index.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 a7cf419...bb6dfa2. Read the comment docs.

[okwolf]

I like the idea, but wouldn't this have a negative impact on testing your app? You wouldn't be able to assert on any VDOM subtree that used a component since it would just be the function and props instead of expanding into the resulting VDOM tree?

[frenzzy]

For tests we can use something similar to react's test-renderer

import TestRenderer from 'hyperapp-test-renderer'

const testRenderer = TestRenderer.create(<Component />)
expect(testRenderer.toTree()).toBe(theWholeExpectedTreeHere)

[jorgebucaran]

@frenzzy Why did you remove this test? Please add it back.

[frenzzy]

This test was moved from h.test.js to components.test.js because the check for null, true and false was moved from h to app runtime.

[frenzzy]

or maybe

return getVNode(node.nodeName(node.attributes, node.children, globalState, wiredActions))

to make component signature like this:

const Component = (attributes, children, state, actions) => <div {...props}>{children}</div>

[etalisoft]

(attributes, children, state, actions) => ... was my first inclination on the component signature -- it's actually the signature that hyperapp-connect used. This signature also makes it a little bit easier for userland HOA/HOC solution development. That signature would obviate the need for #604.

[frenzzy]

A crazy idea: what if we will create an internal context object:

export function app(state, actions, view, container) {
  // ...
  var context = { state: globalState, actions: wiredActions }
  // ...
}

and will provide it to all components as this

return getVNode(node.nodeName.call(context, node.attributes, node.children))

so, component signature will stay the same but for higher-order components we provide access to internal context which we can use for implementing connect or Provider/Consumer patterns..

[jorgebucaran]

Nope. 😄

[Swizz]

Interesting idea, I see on this feature an entry point to allow lifecycle event on Components too.

Do we know the performance impact ?

[jorgebucaran]

Before I worry about the perf impact, I worry about how this changes the way we build apps, etc. I need to think about this a lot. 😅

[SkaterDad]

This looks pretty neat. Can someone post some pseudo-code of the new possibilities this creates?

[Mytrill]

Is it me or would this PR allows to implement @zaceno 's hyperapp-context right into core for no performance penalty?

[zaceno]

@Mytrill

There's an inescapable (but small, I think) performance hit from maintaining the context boundaries.

Aside from that, yes! This PR should be more efficient than the "postprocessing" of the vtree I do in my HOA, because I basically have to walk through the entire tree one extra time before every render -- whereas here, it happens during the walkthrough we do anyway.

No, sorry -- it does not help. Proper context implementation requires resolving the vtree before it's passed to the patch function still. At least, I don't see how I could do it any other way. Someone smarter than me might though 😅

[jorgebucaran]

The way this is implemented conflicts with a relatively recent feature, the "ability" to call actions within the view function (/cc @lukejacksonn). A good example of this can be seen in the router here where replaceState is called, which then calls an action thanks to this other code here.

The way this is implemented means that an action can be called before patch returns, which will definitely have unwanted side effects, as well as break the lifecycle callback stack.

A solution is going back to not allowing calling actions inside the view function.

[zaceno]

@jorgebucaran The way this is implemented means that an action can be called before patch returns, which will definitely have unwanted side effects, as well as break the lifecycle callback stack.

Aha! Good point. That's definitely a good argument to process lazy nodes before patch, despite the penalty. (Actions in views is a much more important feature IMO). If only there were a way to have both... 🤔

[jorgebucaran]

@zaceno I think that's one of the basic points of React Fiber. They solved the problem.

[zaceno]

@jorgebucaran interesting. I haven't looked into React Fiber yet (never looked at React's code at all actually). What was the trick that let them solve the problem (if you know) ?

[jorgebucaran]

@zaceno It's described more or less here. I haven't looked much at React's code either, that's why I made Hyperapp right? 😉

---

If we want to introduce this idea of "lazy" components, we'll need to go back to not allow actions in the view. Or call this off.

[zaceno]

@jorgebucaran Thanks for the link! It gave me an idea:

I wonder if we could get around the conflict between lazy components and calling actions in views, if we separate the process like how virtual-dom (and React Fiber, from the looks of it) does things, by first computing the diff to apply (reconciliation), and then in a separate step applying the diff (the rendering). 🤔

Even if that works... I realize it's quite a long way off. So I think yeah, for now, lets call off lazy components. Actions in views i way more important I think.

[frenzzy]

Why do you need to call actions inside a view? Can you provide an example? I can't imagine an example when you need to use actions not from handlers or life cycle events.

Also locking action calls during patching is just a one boolean variable.

[jorgebucaran]

I am not a big fan of the idea either, but that's how we implemented the router. I would be happy to remove this feature and think of other ways to move forward. See my comment here.

[zaceno]

@frenzzy Also fetching data on route, like here: https://github.com/lukejacksonn/hyperapp-fetch

Correct me if I'm wrong, @jorgebucaran, but this conflict extends beyond just calling actions directly in the view, but also to calling actions in lifecycle events, right?

There may be actions you wish to call when a component is created or updated (I can't think of an example, but there are...). Calling the action immediately in the component/view rather than the lifecycle method means the app doesn't have render the whole app once with instantly old state, and then immediately render again. That's also a use case

[jorgebucaran]

...calling actions in lifecycle events, right?

No, why? 🤔

We use the lifecycle callback stack to call all the events after patch returns.

[SkaterDad]

A solution is going back to not allowing calling actions inside the view function.

That's a good thing 😄 . I know this debate was had in the past, and I still 100% believe that keeping the view function "pure" without side effects has great benefits. It's part of the reason I don't use the new router.

All of these PR's lately are a bit hard to follow. Great to see so much enthusiasm!

[zaceno]

@etalisoft Nice codepen! Cool use of the render attribute to get around the current lack of lazy components!

After some thinking, I believe you're right about multiple apps. If there is a Provider/Consumer pair of singletons, two app's should be able to import them without writing over eachother's context.

However it's important that you only use a Provider once in the tree of your app. The problem I still see is maintaining "context boundaries". If you need to create a more specialized context with more/refined data for a part of your app, you'd need to use a Provider2/Consumer2 pair.

That's a line of thinking I haven't been down, because I was always thinking in terms of having a single mechanism for setting/adding to the context, but only make it apply to the descendants of where the context is set. (That's how hyperapp-context works)

Simply having multiple providers gets around that problem. At the cost of having to export a pair of Provider/Consumer components for every branch of context-specialization you need.

[zaceno]

@etalisoft et al. What I wrote above was rambling and wierd. I apologize. I'd delete it but for the record I'm leavign it.

Here's a fork of @etalisoft's codepen, which illustrates the basic problem with the Provider/Consumer pattern.

https://codepen.io/zaceno/pen/vdzwJr?editors=0010

I'm using the exact same implementation for creating Provider/Consumer components -- the only difference is how I compose them in the views.

Notice how it makes no difference wether the provider wraps the component tree or not. It doesn't matter if it's called higher up in the tree than the consumer even. As long as it's called before the consumer in top-down depth-first order, it still works.

[jorgebucaran]

@zaceno What's your point? We want lazy components, don't we? I am really excited about all the new room for activities once this is merged.

[frenzzy]

@etalisoft if you will use createContext pattern as it is in your example then it will introduce the most difficult to catch bugs that you have ever seen because of sharing memory.

Imagine node.js app, where each user request processed independently and for each request Provider component will be executed. Data will be written to the same value variable shared between all requests. And you can't control the order of write/read to this variable, its value become inconsistent. It may work fine for 999 of 1000 requests which makes it impossible to debug and catch the bug.

In reactjs they additionally use internal context object which is unique for each app instance to write/read data produced by Provider/Consumer type of components. See facebook/react#11818

[zaceno]

@jorgebucaran Well, as far as I understand the idea of a Provider, is that only Consumers that are descendants of the provider, should have access to what it provides. That is not the case in this implementation (or any implementation I've seen yet -- except hyperapp-context).

[jorgebucaran]

@zaceno Do you mean this?

[zaceno]

@jorgebucaran Yes that's a version of it. Specifically I was referring to the implementation of Provider/Consumer in @etalisoft 's codepen above. If you look at the fork I made (also linked above) you'll see that it makes no difference where the provider is called, as long as it's before the Consumer in lexical order. I'm quite sure that's not how you want a Provider / Consumer pair to work. And will certainly lead to bugs if you start mixing multiple providers.

[Mytrill]

@SkaterDad @jorgebucaran I have created a small snippet for the router where there is no action call in the view when fetching data in response of a URL change: https://github.com/hyperstart/hyperapp-recipes/blob/master/router/index.js

Is this the kind of implementation you are using, SkaterDad?

[zaceno]

@jorgebucaran We want lazy components, don't we? I am really excited about all the new room for activities once this is merged.

Not sure...

I'm not opposed. I've verified that hyperapp-context is still possible with lazy components, but at an additional small performance penalty.

Yet I don't really see how it benefits me either.

[jorgebucaran]

@zaceno Possible without lazy components, but you need a render attribute in your component. It doesn't work out of the box.

[zaceno]

@jorgebucaran ? - I think you misunderstood me.

I meant that https://github.com/zaceno/hyperapp-context can be updated to keep working if we merge lazy components. I was worried it would be permanently broken with no way of implementing context properly on top of hyperapp. I have now verified that is not the case. I'm not opposed to merging this. I'm just not sure I see the gains.

but you need a render attribute in your component. It doesn't work out of the box.

Correct. It is possible to make components lazy "in userland" by using a render prop. I think that would be preferable, since it gives userland control over what we pass into the lazy components -- so hyperapp doesn't have to have an opinon. And it makes laziness opt-in.

... so ok. A teensy bit opposed.

[jorgebucaran]

@zaceno If we don't make components lazy by default, then it's impossible to share the global state and actions with components. That's the other part of this feature.

But yeah, I am glad we understand each other now.

[zaceno]

@jorgebucaran @etalisoft demonstrated above how it is possible to share global state and actions with all components -- without lazy components --, using a render prop like you just mentioned

[jorgebucaran]

@zaceno Alright, but I mean by Hyperapp, default and out of the box.

const Subview = state => props => <h1>{state.title}</h1>

app(state, actions, () => <Subview />, ...)

or

const Subview = props => <h1>{props.title}</h1>

app(
  state, 
  actions, 
  () => (
    <div>
      {state => <Subview title={state.title} />}
    </div>
, ...)

[jorgebucaran]

@zaceno Sorry, I could've avoided this confusion by saying that I meant so from the beginning. 😅

[zaceno]

@jorgebucaran Ah I see. Ok well my personal opinion on that (I know it's contentious) is:

• it is not useful to me
• it is too opinionated of hyperapp to do that particular thing
• it doesn't break anything for me, so: go ahead if you want 😄
• if you do go ahead, it would be a lot nicer if regular generic components could keep their current signature. I e, both of these should be valid ways of defining a component:

const GenericComponent = (props, children) => <div class={props.foo}>{children}</div>

const ConnectedComponent = (props, children) => (state, actions) => <button onclick={actions.do} disabled={state.possible}>Yo!</button>

//alternatively

const ConnectedComponent = (props, children, state, actions) => <button onclick={actions.do} disabled={state.possible}>Yo!</button>

I also know I've waffled a bit on what my opinions actually are. So... sorry. For now, this is where I stand ;)

[jorgebucaran]

@zaceno I think you can have all the signatures without interference from this feature. I'll give you a proper answer when I get a chance.

[jorgebucaran]

@frenzzy @zaceno Okay, this is what I have in mind.

const Subview = props => state => <h1>{state}</h1>

const view = () => {
  return (
    <div>
      <Subview />
      <h1>Title</h1>
    </div>
  )
}

This signature: (props => (state, actions) => VNode is backward compatible and lets you do this too:

const Subview = props => <h1>{props.title}</h1>

const view = () => {
  return (
    <div>
      {state => <Subview title={state} />}
      <h1>Title</h1>
    </div>
  )
}

[SkaterDad]

@jorgebucaran This signature: (props => (state, actions) => VNode is backward compatible and lets you do this too:

I'm 100% liking this API for components. I don't see any reason to have React-style context since we don't have stateful components, but reducing boilerplate by passing the full state/actions to every component is nice and simple. Nice bonus that it's backwards compatible, too 👍

[jorgebucaran]

@frenzzy Does it make sense to support vnodes as a function for the root? It seems pointless since you already have the state and actions provided by the view.

What's the point of this?

const App = props => (state,actions) => <...>
const view = (state, actions) => <App />

I think this is more logical.

const App = props => (state,actions) => <...>
const view = App(...)

[frenzzy]

@jorgebucaran

const view = () => <Layout>...</Layout>
const view = () => <Router>...</Router>
const view = () => <Provider>...</Provider>
// etc.

[jorgebucaran]

Thanks, @frenzzy! It's merged. I'll be making some changes (components will not be lazy by default) and push up in a few hours. 🎉