Runtime-modifiable lifecycle events

[jeswin]

Initial discussion at https://github.com/forgojs/forgo/issues/54

Initial proposal by @spiffytech

function MyComponent() {
  return (component: ComponentBuilder) => {
    component.mount(() => {
      const interval = setInterval(doStuff, 1_000);
      component.unmount(() => clearInterval(interval));
      const socket = makeWebsocket();
      component.unmount(() => socket.close());
    });
    component.render(() => <div>Hello world</div>);
  };
}

This style feels good to me. What do we get from nesting the arrow function within MyComponent?

This style starts to feel a bit like SolidJS, which might be a good direction to go. Their approach seems like a logical evolution of what major frameworks have turned into.

I like this style more than trying to add events on top of the existing monolith object API.

We might need to make some multicast and some not and eat that as a documentation issue. transformAttrs would be the same as render - no point in having two of them, you'd really want (manually?) composed functions instead.

[jeswin]

What do we get from nesting the arrow function within MyComponent?

What's the other way to get to a ComponentBuilder?

In the previous example on #54 we had discussed attaching an unmount event-handler from inside the mount event-handler. But in that example, the mount event-handler itself (as well as render) isn't bound at runtime. With the ComponentBuilder approach we treat everyone the same - all events (including mount, unmount, render etc) are bound at runtime.

[jeswin]

Alternative, which I think is a bit cleaner:

function MyComponent() {
  const component = new Component();
  component.mount((props, args) => {
    const interval = setInterval(doStuff, 1_000);
    component.unmount(() => clearInterval(interval));
    const socket = makeWebsocket();
    component.unmount(() => socket.close());
  });
  component.render((props, args) => <div>Hello world</div>);
  return component;
}

[jeswin]

Or we keep the current system, but expose the component (and event handlers) via args.

Here's an example which uses your original snippet with minor modifications.

const MyComponent = () => {
  let socket;
  return {
    mount(_props, args) {
      const interval = setInterval(doStuff, 1_000);
      args.component.unmount.push(() => clearInterval(interval));

      socket = makeWebsocket();
      args.component.unmount.push(() => socket.close());
    },
    render() {...}
  }
}

args.component can be destructured, so won't be so bad ergonomically.

const MyComponent = () => {
  let socket;
  return {
    mount(_props, { component }) {
      const interval = setInterval(doStuff, 1_000);
      component.unmount.push(() => clearInterval(interval));
    }
  }
}

Another alternative, which is almost exactly what you proposed earlier. This avoids exposing the event-handler array, but we'll have to figure out how we can detach an event-handler from an event.

const MyComponent = () => {
  let socket;
  return {
    mount(_props, { component }) {
      const interval = setInterval(doStuff, 1_000);
      component.unmount(() => clearInterval(interval));
    }
  }
}

Compared to what I proposed earlier, what I like about these approaches is that component (builder) doesn't need to be instantiated. Saves two lines, and there is no need to import "Component".

function MyComponent() {
  const component = new Component(); // NOT NEEDED
  return component; // NOT NEEDED
}

[jeswin]

However, what I like in what I proposed earlier is that everything happens at runtime. In the latest examples, we construct the component with the static object based structure that we currently have in forgo, and then allow runtime modifications. That may be a cause for confusion.

[jeswin]

Or allow our static object definitions to be a simple event handler, or to be an array.

function MyComponent() {
  return {
    mount: (props, args) => {
      const interval = setInterval(() => console.log("hello"), 100)
      args.component.unmount.push(() => clearInterval(interval));
    },
    unmount: [],
    render: (props, args) => {
      return <div>Hello world</div>
    }
  }
}

If unmount doesn't need to be an array:

function MyComponent() {
  return {
    mount: (props, args) => {
      const interval = setInterval(() => console.log("hello"), 100)
      args.component.unmount = () => clearInterval(interval);
    },
    render: (props, args) => {
      return <div>Hello world</div>
    }
  }
}

[jeswin]

My view has changed. We may not need to do anything except add a "component" property to args. Then the component (and its lifecycle properties) can be modifed at will.

Here's example code from the runtimeLifecycleEvents.tsx test (from the #60 PR):

function ChildComponent() {
  return {
    mount(props: any, args: ForgoRenderArgs<{}>) {
      args.component.unmount = () => {
        state.hasUnmounted = true;
      };
    },
    render() {
      return <div>Hello world</div>;
    },
  };
}

Now what about multiple lifecycle hooks, as you had given in your example? We can do this outside core. The function we'd attach to say 'unmount' could handle multicast handlers.

For example:

multicastEvents(component.unmount, () => { console.log("added"); });
multicastEvents(component.unmount, () => { console.log("added another"); });

In the example above, multicastEvents() could attach a single function to component.unmount which internally manages the array of event handlers.

[jeswin]

Related PR. #60

[jeswin]

The key piece in the PR is this line added:

export type ForgoRenderArgs<TProps> = {
  element: ForgoElementArg;
  update: (props?: any) => RenderResult;
  component: ForgoComponent<TProps> // ADDED NOW
};

[spiffytech]

Re: PR:

I thought about proposing something like this, but I discarded the idea for a few reasons:

1. The goal is to create an API that composes easily, but this approach is by default does not compose, and punts composability to external libs.
2. It creates a footgun: the stock API is last-assign-wins, and if the programmer doesn't diligently Do The Right Thing they'll get hard-to-spot bugs where they erased previous listeners.
3. Retaining both the existing monolith lifecycle methods, plus the assignable functions, seems confusing. Which should you use, and when? If you have an unmount() method and also assign args.component.unmount, does one of those just never get called? Do you now have to be aware that because some 3rd-party library assigns to args.component.unmount, you can't use the unmount() method? Do you and third party libs have to agree on what composition technique/lib you're using?
4. Am I reading correctly that args.component is the object returned by the ctor? Could we avoid a PR for this approach by just having components reference this while using non-arrow functions?

The only advantage I see to this approach is that we can implement it quickly. But it seems suboptimal at achieving the goal.

---

The approach you posted in this comment is more what I was thinking of at the end of #54:

function MyComponent() {
  const component = new Component();
  component.mount((props, args) => {
    const interval = setInterval(doStuff, 1_000);
    component.unmount(() => clearInterval(interval));
    const socket = makeWebsocket();
    component.unmount(() => socket.close());
  });
  component.render((props, args) => <div>Hello world</div>);
  return component;
}

Or since new Component() is boilerplate that every component needs, we could pass it in args:

function MyComponent(_props, args) {and
  args.component.mount((props, args) => {
    const interval = setInterval(doStuff, 1_000);
    args.component.unmount(() => clearInterval(interval));
    const socket = makeWebsocket();
    args.component.unmount(() => socket.close());
  });
  args.component.render((props, args) => <div>Hello world</div>);
}

---

This avoids exposing the event-handler array, but we'll have to figure out how we can detach an event-handler from an event.

Can you clarify what's happening that we need to disconnect individual lifecycle events? (vs just throwing away all listeners after the component unmounts)

[jeswin]

Good points. Among those, this is probably the biggest problem: Do you now have to be aware that because some 3rd-party library assigns to args.component.unmount, you can't use the unmount() method? Do you and third party libs have to agree on what composition technique/lib you're using?

Am I reading correctly that args.component is the object returned by the ctor? Could we avoid a PR for this approach by just having components reference this while using non-arrow functions?

My view is that the this pointer in JS is very confusing: a) A parameter representing the this pointer is less ambiguous. b) It can be anything. When I see a this pointer, I often wonder - what is it? c) A bit of a misfit for our style of code. But kinda ok if we used OO.

So let me withdraw this PR.

Among the two approaches mentioned at the end of your last response, I am in favor of:

function MyComponent() {
  const component = new Component();
  component.mount((props, args) => {
    const interval = setInterval(doStuff, 1_000);
    component.unmount(() => clearInterval(interval));
    const socket = makeWebsocket();
    component.unmount(() => socket.close());
  });
  component.render((props, args) => <div>Hello world</div>);
  return component;
}

Because:

• A function taking just the props is a clean expression. In fact, IDE type checking (at least in earlier versions) got confused if it weren't so.
• A component constructor returning a component seems logical, if though it adds a couple of lines.

If we want to avoid the import of Component from forgo, we could do this:

function MyComponent() {
  return (component: ComponentBuilder) => {
    component.mount(() => {
      const interval = setInterval(doStuff, 1_000);
      component.unmount(() => clearInterval(interval));
      const socket = makeWebsocket();
      component.unmount(() => socket.close());
    });
    component.render(() => <div>Hello world</div>);
  };
}

[spiffytech]

Good points on this. I agree, we ought to avoid it where possible.

---

I prefer the new Component() style over the arrow function style. It's clear and straightforward what's happening, and the arrow function style creates ambiguity as to which scope is the idiomatic one to put closured variables into.

A couple of thoughts:

1. Do we still pass args to all of these functions, or should the stuff on args be moved onto the component object? I've been mulling over how we could lift args.update() out of the lifecycle methods and into the closure scope. Technically that's an separate feature discussion, but if we're changing the semantics here I wanted to bring it up. Reasoning: Partly keeping the render function strictly about rendering, partly to make it simple to use logic that needs a rerender in both event handlers and lifecycle methods, and partly for the performance boost of not recreating event handlers every render (I haven't measured the boost for Forgo, but the perf hit is a common trope in React land).
2. I know I just wrote about the non-composability of function assignments, but it occurs to me that that could be a way to distinguish multicast vs singleton lifecycle handlers. We could do component.thing1(() => ...) for multicast, and component.thing2 = () => ... for singlecast. The singlecast methods I think avoid all of the caveats from my last past. Thoughts? (Apologies for the whiplash on this, it's been a bit of a day over here.)

Here's what I interpret this whole proposal looking like in a more fleshed-out component, incoporating stuff mentioned here in conjunction with hypothetical / stand-in implementations of contexts & attr transformers. This'll let us see how we feel about the syntax in a larger example, with everything put together.

Lemme know what you think about all this. So far I feel good about this proposal.

const myProjectComponentFactory = () => {
    const component = new Component();
    // Using assign-style here, since we think this wouldn't make sense with multicast
    component.transformAttrs = (key: string, value: unknown) => {
        if (!key.startsWith('on')) return value;
        return () => component.withErrorBoundary(component, captureExceptions(autoRedraw(component, value)))
    }
    return component;
}

const setupWebsocket = (component: Component) => {
    const socket = mkSocket();
    // Setting up a lifecycle method inside another lifecycle method
    component.unmount(() => socket.close());
    return socket;
}

const setupInterval(component: Component, cb: () => void, period: number) => {
    component.mount(() => {
        const interval = setInterval(cb, period);
        component.unmount(() => clearInterval(interval))
    })
}

const MyComponent = (_props) => {
    // This is how I'd expect users to apply standardized component
    // patterns across their whole project
    const component = myProjectComponentFactory();

    let valuePulledFromContext: string | null = null;
    let myVariable: unknown[] | null = null;
    let someValue = true;
    const fetchData = async () => {
        myVariable = await doStuff(valuePulledFromContext!);
        component.update();
    }

    // Having `update()` on `component` instead of `args` lets us put the handler
    // out here, only creating it once
    const onClick = () => {
        someValue = !someValue;
        component.update();
    }

    let socket;
    component.mount(async (_props) => {
        socket = setupWebsocket(component);
        await fetchData();
    })
    setupInterval(component, fetchData, 1_000);

    // Testing out the assign style for non-composable lifecycle functions
    component.render = (_props) => {
        // Not sure how I feel about putting the lookup in here. But render() is
        // the only lifecycle method that gets called repeatedly, so it's where
        // retrieving a dynamic value would have to go (unless we abuse shouldUpdate).
        valuePulledFromContext = libcontext.lookup(component, 'foo');

        if (!myVariable) return <div>Loading...</div>;
        return <>
            <p>Hello, world! The data is {JSON.stringify(someValue)}</p>
            <ul>
                {myVariable.map(value => <li>{value}</li>)} 
            </ul>
            <button type="button" onclick={onclick}>Click me!</button>
        </>
    }
}

[jeswin]

I like this proposal (as seen in the code example). I think we're pretty close to an agreement here.

I've been mulling over how we could lift args.update() out of the lifecycle methods and into the closure scope. Technically that's an separate feature discussion, but if we're changing the semantics here I wanted to bring it up.

I think it's a very good idea. component.update() is more meaningful that args.update().

partly to make it simple to use logic that needs a rerender in both event handlers and lifecycle methods, and partly for the performance boost of not recreating event handlers every render

Not sure if I entirely followed this. (Irrespective, we should move update() into component).

Minor: In the code example, I think you missed returning component from MyComponent() - just making sure this is what you meant.

[jeswin]

We could do component.thing1(() => ...) for multicast, and component.thing2 = () => ... for singlecast. The singlecast methods I think avoid all of the caveats from my last past.

Can you provide an example? I am not sure if I fully understood this.

Should we we support multicast and singlecast?

• Is component.render singlecast, while component.mount supports multicast? (from the code example)
• If component.render is also multicast, do we take the last handler's output, or do we merge the output from all handlers?
• It might be a little confusing to have multicast and singlecast, rather than say that all events are multicast.

Should we use an array and expose it?

component.mount.push(async (_props) => {
    socket = setupWebsocket(component);
    await fetchData();
});

Advantages: 1) handlers can be removed [1]. May not be useful for everyone, but could be for extension authors. I can think of a few usecases (such as in aiding debugging/logging). And FWIW, desktop GUI frameworks usually allow you to do this.

[1]: Removing handlers

component.mount = component.mount.filter(fn => fn !== someFunc);

[jeswin]

To make it more relatable and familiar, perhaps we should use addEventListener() and removeEventListener() which are familiar to web devs.

component.addEventListener("mount", (async (_props) => {
  socket = setupWebsocket(component);
  await fetchData();
}));

Or use the very similar NodeJS Emitter semantics.

component.on("mount", (async (_props) => {
  socket = setupWebsocket(component);
  await fetchData();
}));

But given that this is purely a browser framework, addEventListener() and removeEventListener() might be the way to go.

Other considerations:

• Event Listening is appropriate for "mount", "unmount" etc, while render is like a method().
• The type of "handler" in addEventListener(type, handler) will need to be uniform. This would have been a problem earlier, but now our handlers are actually of the same type (they just accepts props).

[jeswin]

Event Listening is appropriate for "mount", "unmount" etc, while render is like a method().

I think a clear separation of what are event listeners, and what are methods could be important. Mount, unmount etc are events/listeners, while render and shouldUpdate are methods.

Updated proposal:

const myProjectComponentFactory = (methods) => {
  const component = new Component(methods);
  // Using assign-style here, since we think this wouldn't make sense with multicast
  component.transformAttrs = (key: string, value: unknown) => {
    if (!key.startsWith("on")) return value;
    return () =>
      component.withErrorBoundary(
        component,
        captureExceptions(autoRedraw(component, value))
      );
  };
  return component;
};

const setupWebsocket = (component: Component) => {
  const socket = mkSocket();
  // Setting up a lifecycle method inside another lifecycle method
  component.on("unmount", () => socket.close());
  return socket;
};

const setupInterval = (
  component: Component,
  cb: () => void,
  period: number
) => {
  component.on("mount", () => {
    const interval = setInterval(cb, period);
    component.on("unmount", () => clearInterval(interval));
  });
};

const MyComponent = (_props) => {
  function render(_props) {
    // Not sure how I feel about putting the lookup in here. But render() is
    // the only lifecycle method that gets called repeatedly, so it's where
    // retrieving a dynamic value would have to go (unless we abuse shouldUpdate).
    valuePulledFromContext = libcontext.lookup(component, "foo");

    if (!myVariable) return <div>Loading...</div>;
    return (
      <>
        <p>Hello, world! The data is {JSON.stringify(someValue)}</p>
        <ul>
          {myVariable.map((value) => (
            <li>{value}</li>
          ))}
        </ul>
        <button type="button" onclick={onclick}>
          Click me!
        </button>
      </>
    );
  }

  function shouldUpdate() {
    return Math.random() > 0.5;
  }

  // This is how I'd expect users to apply standardized component
  // patterns across their whole project
  const component = myProjectComponentFactory({ render, shouldUpdate });

  let valuePulledFromContext: string | null = null;
  let myVariable: unknown[] | null = null;
  let someValue = true;

  const fetchData = async () => {
    myVariable = await doStuff(valuePulledFromContext!);
    component.update();
  };

  // Having `update()` on `component` instead of `args` lets us put the handler
  // out here, only creating it once
  const onClick = () => {
    someValue = !someValue;
    component.update();
  };

  let socket;
  component.on("mount", async (_props) => {
    socket = setupWebsocket(component);
    await fetchData();
  });
  setupInterval(component, fetchData, 1_000);

  // Testing out the assign style for non-composable lifecycle functions
};

[jeswin]

An alternative proposal, if you don't like render() and other methods/listeners referring to component which is outside its body is to add it as a second parameter:

function render(props, component) {
  return (<div>
    <button onclick={() => component.update()}>Click me!<button>
    <p>hello world</p>
  </div>)
}

Advantages:

• I generally prefer this style, because it makes the functions purer. And not bound to the closure.
• They are easier to unit test
• They can even be reused (since they're aren't coupled to the surrounding scope) [1]

Other considerations:

• JS allows a function body to refer to a variable which was not available when the function was created. The original code example works just fine.
• If we pass the component into render(), we should do it for mount and unmount handlers as well.

[1]: Reuse of handlers

function render(props, component) {
  // ... do stuff
}

function InboxComponent() {
  const component = myProjectComponentFactory("InboxComponent", { render });
  // ... do stuff
}

function logger(props, component) {
  // Print "InboxComponent just mounted"
  console.log(component.name + " just mounted"); 
}

component1.on("mount", logger);
component2.on("mount", logger);

Also, if the second parameter is omitted in the declaration of render/mount/unmount, it can be used in the previous form.

This still works:

function MyComponent(props) {
  function render(props) {
    return (<div>
       <button onclick={() => component.update()}>Click me!<button>
       <p>hello world</p>
    </div>);
  }
  const component = myProjectComponentFactory({ render });
}

[spiffytech]

partly to make it simple to use logic that needs a rerender in both event handlers and lifecycle methods, and partly for the performance boost of not recreating event handlers every render

Not sure if I entirely followed this. (Irrespective, we should move update() into component).

Simple logic part: in Forgo's current API, imagine you have a data fetch function you want to run on mount, and after some click events. You'd like to declare that function it the closure scope, but in Forgo's current API, that scope doesn't have access to args.element so it can't include the call to rerender(). So you have to come up with workarounds.

Performance part: the idea behind React's useCallback is that, in tight render loops, the cost of redeclaring click handler functions every single render starts to affect performance, so you look for ways to avoid doing that. Lifting args.update() up to component.update() makes that more natural to do in Forgo. Does that perf difference matter all the time? Probably not. But it's a nice bonus if the change improves ergonomics anyway.

Minor: In the code example, I think you missed returning component from MyComponent() - just making sure this is what you meant.

Oh, yep! I meant to return it.

We could do component.thing1(() => ...) for multicast, and component.thing2 = () => ... for singlecast. The singlecast methods I think avoid all of the caveats from my last past.

Can you provide an example? I am not sure if I fully understood this.

Looks like you got on the same train of thought with this comment:

I think a clear separation of what are event listeners, and what are methods could be important.

In the code example I gave, you'll note that I did component.render = ..., but component.mount(() => ...)., to distinguish things that are multicast vs things we want to signal aren't meant to compose. I like your decisions better.

---

I like the the choice of methods + component.addEventListener(). The event emitter does imply we'd have a component.removeEventListener(), which I don't see a use case for, but leaving it out could be confusing. But maybe not confusing enough to worry about?

Is shouldUpdate() actually unicast? I could see a state library wanting to hook into shouldUpdate(), while the developer also wants to use it. Maybe multicast, with semantics of "component will update if any listeners return a truthy value"?

An alternative proposal, if you don't like render() and other methods/listeners referring to component which is outside its body is to add it as a second parameter

I like passing component to events/methods, regardless of our choice for the closure scope ergonomics.

Re: hoisting:

Pro: I like that the new Component(methods) syntax makes it really easy to type check that we get a well-formed component (i.e., mandatory render method).

Con: hoisting is probably JavaScript's most unintuitive feature; it's tricky to understand what it really does, and makes it harder to read/understand your own code. Plus it opts out of TypeScript's control-flow type inference.

Anything in the closure scope that needs to rerender needs access to the constructed component, and it either gets declared first and hoists the component, or gets declared second and gets hoisted by the component. Both of those feel kinda icky.

Re: naked render purity / testability / reusability: will we get this in the wild? I'm not sure when I'd want a standalone render() function that didn't depend on closure scope for functions/state, without having a reason to make a new component instead. I'm just thinking through how much weight to give optimizing for this.

What about moving method declaration out of the new Component() call and into the ctor's return value, by having the ctor return a tuple? This example avoids hoisting, preserves type safety in what the ctor returns, and still leaves render() as standalone if some case benefits from that.

function MyComponent() {
  const component = new Component();
  component.mount(async (_props, component) => {
    await fetchData();
    component.update();
  });

  let data;
  async function refresh() {
    data = await fetchData();
    component.update();
  }

  const componentMethods: forgo.ComponentMethods = {
    render() {
      return <>
        <button onclick={refresh}>Refresh</button>
        <div>{data}</div>
      </>
    }
  };

  return [component, componentMethods];
}

That feels... a little weird? To say the component isn't the whole component? Right now I'm not sure of another way to achieve a type safe return while also dodging the hoisting.

[spiffytech]

Maybe we can use the spread operator to assign methods to the component?

function MyComponent() {
  // changed from `new Component()` since it now returns an incomplete type
  const component = forgo.createComponent()
  component.mount(async (_props, component) => {
    await fetchData();
    component.update();
  });

  let data;
  async function refresh() {
    data = await fetchData();
    component.update();
  }

  return {
    ...component,
    render() {
      return <>
        <button onclick={refresh}>Refresh</button>
        <div>{data}</div>
      </>
    }
  };
}

If forgo.createComponent() returns Omit<Component, 'render'>, then we force the ctor to fill in the mandatory method, and we get one logical component object.

[spiffytech]

Or we go with a callback?

function MyComponent() {
  return new Component((component => {
    component.mount(async (_props, component) => {
      await fetchData();
      component.update();
    });

    let data;
    async function refresh() {
      data = await fetchData();
      component.update();
    }

    return {
      render() {
        return <>
          <button onclick={refresh}>Refresh</button>
          <div>{data}</div>
        </>
      }
    };
  });
}

Disadvantage: now we're back to two closure scopes.

Idunno. All four of these (hoisting, tuple, spread, callback) work, but none of them feel right.

[jeswin]

Right now I'm not sure of another way to achieve a type safe return while also dodging the hoisting.

Sorry I may be missing something. If the signature of render is render(props, component), we don't need to worry about hoisting right?

The component we're referencing inside render() isn't captured from surrounding scope; instead it's passed in by forgo. To make it clear, I've called the constructed component myNewComponent in the following example.

function MyNewComponent(props) {
  function render(props, component) {
    return <button onclick={() => component.update()}></button>;
  }

  // Assume this is unicast for now (will fix)
  function shouldUpdate() {
    return true;
  }

  // OR const myNewComponent = myAppComponentFactory({ render, shouldUpdate });
  const myNewComponent = new Component({ render, shouldUpdate });

  myNewComponent.addEventListener("mount", () => console.log("mounted"));

  return myNewComponent;
}

[spiffytech]

render() doesn't need to hoist access to the Component instance, but it does need to hoist access to userland methods that depend on the Component instance.

E.g., if there's a click handler declared in the closure scope that rerenders, you either need: to hoist the click handler

function MyComponent() {
  const component = new Component({
    // Hosting the onclick handler
    render() {
      <button onclick={onclick}>Do Stuff</button>
    }
  });

  const onclick = () => {
    ...
    component.update();
  }
}

Or, you need to hoist the component:

function MyComponent() {

  // Hosting the component
  const onclick = () => {
    ...
    component.update();
  }

  const component = new Component({
    render() {
      <button onclick={onclick}>Do Stuff</button>
    }
  });
}

[jeswin]

But you could write clickHandler inside render.

function MyNewComponent(props) {
  function render(props, component) {
    function clickHandler() {
      component.update();
    }
    return <button onclick={clickHandler}></button>;
  }

  return new Component({ render });
}

[spiffytech]

But you could write clickHandler inside render.

Ah, sorry. This is what I was getting at at the top of this comment, but I don't think I explained it very well.

Putting event handlers inside render() means those handler's can't be accessed from outside render(). But doing exactly that is a common pattern: fetch some data on mount, maybe poll for updates, and fetch again after clicking a button.

Contrived example with Forgo's existing API:

function MyComponent() {
  let data;
  let doStuff;

  return {
    async mount() {
      await doStuff!();
      setInterval(doStuff!, 10_000);
    },
    render(_props, args) {
      doStuff ||= async () => {
        data = await fetchData();
        args.update();
      }

      return <>
        <button onclick={doStuff}>Do Stuff</button>
        <div>{data}</div>
      </>
    }
  };
}

Here, we have a few different places where we want to fetch data and rerender. Because rerendering requires access to args, the doStuff() function has to be declared inside a lifecycle method, then exfiltrated into the closure context so other lifecycle methods can access it.

If we're already rethinking the semantics of the component API, this is something I'd like to improve. If the user has a function that rerenders and needs to it from multiple places, they should be able to declare it in the closure scope, without making + copying + non-null asserting it. As a bonus, declaring it in the closure scope lets it stay right next to the data it modifies. I also find that my render methods start becoming harder to comprehend when they grow to include both JSX definition + several function definitions inside the render method.

Improved experience:

function MyComponent() {
  let data;
  const doStuff = async () => {
    data = await fetchData();
    // Doesn't work in existing API, could work with `new Component(); component.update()`.
    args.update();
  }

  return {
    async mount() {
      await doStuff();
      setInterval(doStuff, 10_000);
    },
    render(_props, args) {
      return <>
        <button onclick={doStuff}>Do Stuff</button>
        <div>{data}</div>
      </>
    }
  };
}

[jeswin]

Putting event handlers inside render() means those handler's can't be accessed from outside render(). But doing exactly that is a common pattern: fetch some data on mount, maybe poll for updates, and fetch again after clicking a button.

It'd need to accept the component as an arg, but is that a problem?

function MyNewComponent(props) {
  function doStuff(component) {
    console.log("Doing stuff...");
    component.update();
  }

  function render(props, component) {
    function clickHandler() {
      doStuff(component);
    }
    return <button onclick={clickHandler}></button>;
  }

  return new Component({ render });
}

[spiffytech]

...

Yeah, that'll work.

Minor nit that that requires the runtime cost of declaring that click handler function on every render, but if that somehow really becomes a performance problem, I think it's fine to ask users to do the aforementioned ||= approach on an as-needed basis. The important parts are colocation of functions + data, and good types ergonomics, which we get from just passing the component around. I'm not sure that it ever matters, but React land seems to work hard to avoid the extra function declaration, so I figured it was worth thinking about.

---

Summary of the current proposal:

Pretty sure we're on the same page about:

• const component = new Component(); return component
• Component() requires as an argument an object containing non-event-y lifecycle methods
  • render() is mandatory
  • transformAttrs() would eventually go here
• With component instance, event-y lifecycle listeners can be added with component.addEventListener('mount', () => ..). Multiple listeners is okay, all will be executed.
  • We probably want to guarantee that they run in the order in which they were registered.
• An app can register lifecycle listeners from within other listeners, so a mount listener can register its own unmount logic.
• The component instance is expected to be passed around in userland. It is the cornerstone of composing + extending a Forgo component.
• args.update() becomes component.update()
• All events + methods will receive as arguments the props + component instance, in that order

Unclear / undecided:

• The component instance becomes the stable identifier anytime userland needs to interact with Forgo, replacing the userland use of args.element.
  • Stuff like the eventual contexts library would hand the component instance to Forgo to request its parent component, getting back another component instance.
• shouldUpdate() becomes multicast, and component will rerender if any shouldUpdate() listener returns truthy.
• Does args / ForgoRenderArgs disappear from userland entirely?
  • Do we move args.element -> component.element, or remove it altogether since we'll have component.update()?

How does that sound? Lots of back and forth, including me having a braindead morning 🙂

[jeswin]

Unclear / undecided:

The component instance becomes the stable identifier anytime userland needs to interact with Forgo, replacing the userland use of args.element.

I think yes.

shouldUpdate() becomes multicast, and component will rerender if any shouldUpdate() listener returns truthy.

I'm ok with this

Does args / ForgoRenderArgs disappear from userland entirely?

I think it should go.

Do we move args.element -> component.element, or remove it altogether since we'll have component.update()?

We could retain component.element - doesn't seem out of place. But perhaps not discuss it in docs.

[spiffytech]

Sounds good! I'll start looking into implementation later today.

[spiffytech]

I've done a first pass conversion of the framework code and have been porting test cases over to the new syntax. I've discovered the delightful fact that if you annotate the types for the ctor, TypeScript only infers the Props generic for the Component if it's immediately returned, but not if you create a local variable, add event listeners, and then return.

// TypeScript complains that the return type doesn't match the declaration because
// the ctor has props but the component can't figure out it should get its props from the
// ctor's annotation
const StatefulComponent: forgo.ForgoComponentCtor<
  forgo.ForgoComponentProps & { key: unknown }
> = () => {
  let state = getRandomString();
  // Component() is really Component<Props extends ForgoComponentProps>().
  // Without an explicit annotation TS has to deduce what that generic parameter's
  // value is. And it fails to do so if you assign the component to a variable instead
  // of immediately returning it.
  const component = new forgo.Component({
    // TS error here that props doesn't include `key` because the component doesn't
    // pick up the ctor's generic parameter
    render({ key }) {
      savedState.set(key, state);
      return (
        <p state={state} key={key}>
          Component #{key}
        </p>
      );
    },
  });
  // Another TS error here about `key`
  component.addEventListener("unmount", ({ key }) => {
    savedState.delete(key);
  });
  return component;
};

// This syntax typechecks just fine because TS can tell that the return value needs to
// conform to the ctor's type signature. But of course now the user doesn't have a
// variable to add event listeners to
const StatefulComponent: forgo.ForgoComponentCtor<
  forgo.ForgoComponentProps & { key: unknown }
> = () => {
  return new forgo.Component({
    render({ key }) {
      savedState.set(key, state);
      return (
        <p state={state} key={key}>
          Component #{key}
        </p>
      );
    },
  });
};

Right now I see three options:

1. Maybe there's a way to get TS to figure this out and make the whole problem go away. I haven't found that yet. In this case I'm mostly guessing at how I've angered TS and trying incantations to appease it.
2. Tell users to create a named interface for props. Solves the problem, but it doesn't feel great to make named interfaces for every little thing, especially when each one is only used in one spot in your code. 2a. The current test suite doesn't annotate the ctor, and we could just say "don't annotate the ctor" and then the user is only parameterizing the component and there's no duplication. I don't think that's what we should encourage: it leaves room for errors (e.g., all of the unconverted unit tests typecheck right now because TS doesn't know that the API change broke any of them). And since we pass props to the ctor, the user would want type checking there anyway.
3. Maybe TS isn't capable of inferring the generic parameter in this API design? I don't know enough to say. But if we can't make (1) work and we don't like (2), we're left with redesigning the API around TypeScript's limitations.

[spiffytech]

I don't have any concrete thoughts / action steps on this yet, I just want to note it down while it's on my mind:

From an extensibility point of view we need to support multiple shouldUpdate callbacks, but it feels weird to call it an event listener since it has a meaningful return value. But I don't want to go inventing a third way to specify behavior on components.

[jeswin]

Amazing Progress!

Unfortunately, I'm not sure if I fully understood the problem. I mean, I can see the problems but a little confused - I'm wondering if there are multiple issues at play here.

First, we seem to have missed out defining "key" in ForgoElementProps. When I add that, a couple of errors go away. Adding key to ForgoElementProps kinda makes sense, right?

Before I go further (and just to make sure we're on the same page), can you modify the code you posted above so that StatefulComponent also takes in a few props of its own (such as say firstName and lastName)? [1]

1: Just so that I don't take the thread on a different tangent. I'll post a longer reply after looking at the example.

[spiffytech]

Adding key to ForgoElementProps kinda makes sense, right?

I've waffled on that for a while. Right now children have access to key, but I haven't teased out whether to consider it a leaky implementation detail or a useful design decision. We keep taking advantage of it in unit tests and sandboxes, but I don't know whether there's a real-world use case for reading the key used in the parent context.

Adding key to ForgoElementProps comes down to whether reading the parent context key is something we want to support + encourage. To date I've leaned in the direction of not making that an official part of the API until we have a compelling reason to, but I'll follow your lead on it.

---

can you modify the code you posted above so that StatefulComponent also takes in a few props of its own (such as say firstName and lastName)?

Sure thing. That code was copy/pasted out of the first unit test I encountered that needed a props annotation. Here's a modified version that demonstrates the issue a bit more clearly:

const StatefulComponent: forgo.ForgoComponentCtor<
  forgo.ForgoComponentProps & { firstName: string, lastName: string }
> = () => {
  const component = new forgo.Component({
    render({ firstName, lastName }) {
      return (
        <p>
          Hello, {firstName} {lastName} !
        </p>
      );
    },
  });
  return component;
};

The line with const StatefulComponent produces this error:

Type 'ForgoElementProps' is not assignable to type 'ForgoElementProps & { firstName: string; lastName: string; }'.ts(2322)

Because the new forgo.Component() construct is inferred to be a Component<forgo.ForgoElementProps>, while TypeScript expects the ctor to return a Component<ForgoElementProps & { firstName: string; lastName: string;}>.

If the code is modified from const component = new Component(...) ; return component; to instead read return new Component(...), TypeScript can correctly deduce that the component is a Component<ForgoElementProps & { firstName: string; lastName: string;}> because that's what the ctor's type signature says it should return.

Full version that typechecks correctly:

const StatefulComponent2: forgo.ForgoComponentCtor<
  forgo.ForgoComponentProps & { firstName: string, lastName: string }
> = () => {
  return new forgo.Component({
    render({ firstName, lastName }) {
      return (
        <p>
          Hello, {firstName} {lastName} !
        </p>
      );
    },
  });
};

The problem with this version ^^ is that now we don't have access to a component variable to add event listeners to, pass to libraries, etc.

We can solve the issue by supplying a type annotation to both the ctor and to the Component constructor:

interface StatefulComponentProps extends forgo.ForgoComponentProps {
  firstName: string;
  lastName: string
} 

const StatefulComponent: forgo.ForgoComponentCtor<StatefulComponentProps> = () => {
  const component = new forgo.Component<StatefulComponentProps>({
    render({ firstName, lastName }) {
      return (
        <p>
          Hello, {firstName} {lastName} !
        </p>
      );
    },
  });
  return component;
};

It's effective, but verbose and clunky. Declaring the named interface doesn't feel right if it's not going to be used anywhere else in the codebase but right here.

[spiffytech]

Looks like we get the same behavior with the existing API if we assign the component to a variable instead of returning it immediately:

const App: ForgoComponentCtor<
  ForgoComponentProps & { firstName: string }
> = () => {
  // TS complains that `ForgoComponent` needs a type parameter
  const component: ForgoComponent = {
    // TS complains that firstName is implicitly `any`.
    render({ firstName }) {
      return <p>Hello, {firstName}!</p>;
    }
  };
  return component;
};

And we're hitting this because we've never had a reason to assign the component to a variable before.

[jeswin]

I think having to repeat the type argument is fine. It's slightly more verbose, but it's logical and hopefully not too bad. Sorry haven't spent enough time on this yet - but will do over the weekend.

[spiffytech]

Okay, I'll proceed with doing the unit test conversions that way.

[jeswin]

I find the following example of yours quite reasonable - I like it. (I tried a few other approaches, but this is still the most elegant.)

interface StatefulComponentProps extends forgo.ForgoComponentProps {
  firstName: string;
  lastName: string
} 

const StatefulComponent: forgo.ForgoComponentCtor<StatefulComponentProps> = () => {
  const component = new forgo.Component<StatefulComponentProps>({
    render({ firstName, lastName }) {
      return (
        <p>
          Hello, {firstName} {lastName} !
        </p>
      );
    },
  });
  return component;
};

Adding key to ForgoElementProps comes down to whether reading the parent context key is something we want to support + encourage. To date I've leaned in the direction of not making that an official part of the API until we have a compelling reason to, but I'll follow your lead on it.

Alright, let's not do anything on this for now. Hard to walk back once we publish.

[spiffytech]

I've got all the existing unit tests passing against the new API.

Still too:

• Write tests for the new behavior
• Remove some old types like ForgoRenderArgs that we shouldn't be using anymore
• Update documentation
• Make forgo-state etc. compatible with the new API.
• Actually watch the new code run in a browser and not just the unit tests.

At some point I'll publish an alpha release of all the packages so I can start porting my app over and test this stuff in a real project.

[jeswin]

Amazing stuff! Thanks for all your hard work on this - and sorry I wasn't of much help. I'll try it out today.

I think we should make some noise about this release - it's the biggest change since V1. :)

In addition to the things you mentioned, we should:

• improve the performance (I'll look at it this week)
• write a blog about v3, how it's different etc.
• Maybe do a "Show HN" or something.

Also, I can fix up forgo-state this week.

[spiffytech]

I agree, between this and all the other work over the last year, Forgo is stronger now and we should highlight that.

Doing perf work before making the big announcement makes sense. In that case, let's make sure the JS Benchmarks page reflects the new performance before the announcement goes out.

[spiffytech]

I cleaned up old types and the TODOs I had. I also adjusted the component API so that specifying the component props type can be done as new Component<MyProps>() instead of new Component<ForgoComponentProps & MyProps>(), because that's been bugging me forever.

And I added unit tests asserting the shape of the new API - that event listeners get called, in order, that shouldUpdate kicks in if any of the listeners returns true.

I still haven't done a manual test in a sandbox. Besides that, I need to do docs updates. Then it's just updating the extra forgo packages to use the new API and I think this will be ready for review.

I'll cut an alpha soon so that we can start using the new API in sandboxes and forgo-state/forgo-router/...

[jeswin]

That's phenomenal! I'll look at it tomorrow or the day after, just recovering from a mild covid infection.

[spiffytech]

Oh no! Sorry to hear that. I've had several people get it in the last couple weeks.

forgo@4.0.0-alpha.0 (tag prerelease) is now published, and the "hello world" works in a sandbox! I'm going to look at the forgo-state conversion now; I want to get this ready to convert my app over since that'll test it better than any sandbox I think up.

[spiffytech]

Well... that was easy.

I've update forgo-state and forgo-router and cut alphas for them, and ported my app to the new component API, and all of my e2e tests pass. Since the minimum upgrade path is syntax with little semantic difference it was basically all just going as fast as I could type (though I'm not taking advantage of the new structural opportunities the new API enables yet).

For forgo-state I changed it to not return a component, since now it doesn't need to create a wrapper - it can just use the event listeners. The new API doesn't let forgo-state control whether its listeners come before any listeners registered in userland, but I'm not sure that's a problem.

[spiffytech]

Thoughts after converting forgo, forgo-state, forgo-router, my own app:

1. For the most part the API feels good. Sometimes doing return new Component and sometimes const component = ...; component.addEventListener(...); return component is a little bit of friction, but I think it's definitely worth it for what it gets us in extensibility.
2. The more I sit with it, the more it feels incorrect & unintuitive to call shouldUpdate an event listener when it has a return value and Array.some(...) semantics. But I haven't figured out what it should be changed to.
3. Components have a .__internal property that forgo-state and forgo-router need to reach into. Ideally we'd hide that from userland with TypeScript's stripInternal flag, but then our own extension packages couldn't see it. We should figure out what in .__internal makes sense to expose on a public API and hide the rest. I think it's usually the node, but exposing that feels weird since it's an implementation detail.

[spiffytech]

Addendum: transformAttrs would probably also need to be multicast (e.g., wrap click handler in both auto-refresh and error capture), and wouldn't be served by event listener semantics - it'd probably be more like a reducer. Maybe using component.on() instead of component.addEventListener() would minimize confusion? Not sure about that.

[spiffytech]

I keep looking for ways to improve this proposal, but nothing feels great, just okay. Here's what I'm thinking right now:

const MyComponent: ForgoComponentCtor<MyProps> = (_props, {component}) => {
  component.mount.push(() => {
    ...
    component.unmount.push(() => ...);
  });
  component.transformAttrs = () => ...
  component.shouldUpdate.push(() => ...);
  component.render = () => ...
};

Pros:

• The user doesn't need a named interface for props, since they'll be passed component that matches the generic parameter passed to ForgoComponentCtor
• It's very clear whether a method is single-cast or multicast
• Multicast methods don't carry semantic baggage (like being shaped like an event listener). If we need to give any of them special behavior (e.g., shouldUpdate) we can just do that.
• This saves a modest amount of boilerplate: the user doesn't have to instantiate the component, return it, or amend their forgo import to explicitly import Component (if using they're named imports)
• The user can define the component's methods in a logical order. One downside of the existing proposal is that it feels weird to define render before you define the methods that affect what happens in render (mount setting variables, shouldUpdate deciding whether to render at all).
• Minor Forgo code simplification vs the current proposal.

Cons:

• We'll have to add a runtime check to ensure render is defined when the constructor returns. The type system can't guarantee that under this proposal. Probably not a big deal since it's our only mandatory method, and it's hard to imagine a user accidentally forgetting to render.
• We'll have to add runtime checks to ensure the array-typed properties don't get assigned to something different. We can keep that code to a minimum by making component a Proxy and denying any set on the relevant properties.
• Exposing the underlying arrays for multicast properties directly to the user feels inelegant, and means we don't control whether the user pushes to the front or back of the array
  • Though letting them make that choice might possibly be useful? Maybe?
  • Exposing the arrays implies a level of trust inside the codebase. There's no good reason some library should empty out the array or overwrite an index, but we're making it possible to do so. We could use even more Proxys to prevent that if we felt strongly about it.

---

I'd like to add a compatibility layer bridging the old component syntax. This would give users an upgrade path, and let us release this as a non-breaking 3.x, with a deprecation notice that the old syntax will stop working in 4.0. We can let the user set a flag on window that makes Forgo print a warning to the console if the old syntax is used, helping users catch any place they haven't migrated (the flag is to keep the console from being spammed).

[spiffytech]

Or maybe we revisit keeping the existing syntax but making multicast properties use arrays. Looking back at that discussion, my objections can be solved by just deciding that mount and friends are always arrays. We can allow both methods and arrays of methods temporarily as a compatibility layer, then in 4.x support only arrays of methods.

This keeps the simplicity of Forgo's current syntax and minimizes the code changes users have to make.

It has the same downsides of exposing arrays I mentioned in my previous comment, and because the user supplies the object we can't even use Proxys to apply runtime protections. But it's simple and elegant.

If we adopt this, I'd like to continue replacing the args parameter with an instance of the component (which Forgo would add an update method to after the ctor returns).

[spiffytech]

Hmmm, if we continue defining the component as whatever the user returns, we lose the perk of exposing component-level methods in the ctor closure scope. No component.update(), component.withErrorBoundary(() => ...) unless the user grabs them in a lifecycle hook and assigns them to a closure-scope variable.

I've always felt that pattern is rather clumsy, and because it sidesteps type safety there's more room for bugs. I'd like to preserve the current proposal's trait of exposing all that stuff in the ctor scope, but that means we can't let the user hand us a POJO to represent the component.

We could get around this by passing these functions in ForgoCtorArgs, then binding them to the component after the ctor returns. That means there are two ways of calling this stuff (ctorArgs.update() in the ctor closure, or component.update() in a lifecycle method). I don't like that ambiguity, but it may be it's an okay tradeoff? I'm not sure.

[jeswin]

Excellent writeup. I agree with the Pros and Cons mentioned above https://github.com/forgojs/forgo/issues/59#issuecomment-1180615004. I don't think the Cons are a big deal. So we should definitely consider this proposal.

Or maybe we revisit keeping the existing syntax but making multicast properties use arrays. Looking back at that discussion, my objections can be solved by just deciding that mount and friends are always arrays. We can allow both methods and arrays of methods temporarily as a compatibility layer, then in 4.x support only arrays of methods.

We can consider this too.

I'd like to preserve the current proposal's trait of exposing all that stuff in the ctor scope, but that means we can't let the user hand us a POJO to represent the component.

We could get around this by passing these functions in ForgoCtorArgs, then binding them to the component after the ctor returns.

I think I largely understand what you're trying to say. But can you give a short example please?

I think I got it.

[jeswin]

We have a set of proposals here. Maybe we should name/number them?

I'll have a go at it:

• Original (ORIG): https://github.com/forgojs/forgo/issues/59#issuecomment-1107347533 which is a minor modification of current Forgo
• Component Returned by User (CRU): https://github.com/forgojs/forgo/issues/59#issuecomment-1107295201 which is very nearly complete
• Component as Ctor Param (COCP): https://github.com/forgojs/forgo/issues/59#issuecomment-1180615004 proposed yesterday.

Please add others I may have missed.

[jeswin]

What I don't like about COCP is that the coding style looks very mutable/imperative. (That we can't type check the render is due to this imho, although that is just a minor niggle.)

component.render = () => {}

That's not the case in CRU (which you implemented), as well as in ORIG. It's not a big deal, but might seem unappealing to people who prefer a more declarative style.