Crank: Passing context as an argument instead of this

[tschaub]

I wonder if you considered calling component functions with a "context" arg instead of applying the same as this. And for generator components, the props property would be an iterable object.

function *Component({props}) {
  let oldName;
  for (const {name} of props) {
    if (!oldName) {
      yield <div>Hello {name}</div>;
    } else if (oldName != name) {
      yield <div>Goodbye {oldName} and hello {name}</div>;
    } else {
      yield <div>Hello again {name}</div>;
    }
    oldName = name;
  }
}

I haven't dug enough to know if this is true, but I wonder if this would stop people from falling into the "stale props" trap described in the Prop updates section.

I like this in JS, but I think it is easier for people to reason about when it appears in class methods. Maybe there is a reason component functions need to be called with a props object. Or maybe it would be too confusing to have props be iterable.

[brainkim]

You know what, I have woken up in a cold sweat once or twice this past month saying “PROPS NEED TO BE ITERABLE.” It would be really good to know if there’s a more ergonomic API that I could have designed, and if I could do away with this, which a lot of people seem to be having some concerns with.

The props passed into components could definitely be iterable/async iterable, insofar as all that involves is adding a Symbol method to the props object. I imagined this could also make higher-order functions with components prettier somehow even though I never really could flesh out the details.

Ultimately, I have a couple reasons why I didn’t create the API like you suggest, although I definitely considered it:

1. Destructuring

If the props object is also the iterable of props which represents updates, you couldn’t use object destructuring in the component’s parameters:

function *Counter({message}) {
  let count = 0;
  // how do I access props?
  for ({message} of ???) {
    count++;
    yield (
      <div>{message} {count}</div>
    );
  }
}

I know there are solutions, like you could destructure in a variable assignment const {message} = props, or you could even try something like {message, [Symbol.iterator]: updates} for the parameter, but one personal goal I had was to make refactoring a stateless function component to a stateful generator component as few edits as possible, and these solutions seemed just as if not more verbose than iterating over this.

1. TypeScript.

Crank is written in TypeScript, and although I make liberal usage of any I’ve tried my best to make components strongly typed. Thankfully, TypeScript understands function components and that the first argument to the function is the type of the props for elements, but you wouldn’t be able to also then iterate over the props object, because you can’t just iterate over arbitrary objects in TypeScript.

You would have to either 1. cast props to an iterable which is yucky, or 2. use a type like function *Counter({message}: Props<{message: string}>), with the Props type being the intersection of the type parameter and an iterable of the type parameter.

I decided against this 1. because TypeScript support for JSX that is not React is already super tenuous and I wasn’t sure if it would understand the special Props type, and 2. because it felt more verbose, and I think it’s important to save space in the parameter list for actual type information.

1. Components aren’t regular functions.

I know people sorta balk at the idea of a top-level function being called with a this object, but I think this can also be an advantage. It means that Components are special, and they can’t be invoked by anyone, which was a problem with React where people would mistakenly call function components directly in element trees rather than creating elements with them. I think at the end of the day people will get used to a special this for components, and I do hope to make this more useful via some kind of global plugin system which would allow you to, for instance, define a fetch method which aborts when the component is unmounted.

I dunno, talking through this makes me feel more confident but I would also appreciate your thoughts. If there is a time to change it, it’s now.

[brainkim]

Ahhh I’m rereading this and seeing I might have misunderstood your suggestion. You’re saying we put the props in their own key on the object passed to components, so that we can pass other keys. This is an interesting proposal; unfortunately, I don’t think this would work for TypeScript, which expects the props to be the first argument. I dunno if that’s configurable but this is something to think about.

[brochington]

Simply my opinion, but I could imagine something similar to this to be fairly agreeable for most:

// This would mostly likely be defined by Crank
type ComponentArgs<P> = {
  props: P
}

// elsewhere
import { ComponentArgs } from '@bikeshaving/crank';

interface Props {
  something: 'here'
}

export default function MyComponent({ props}: ComponentArgs<Props>): Element {
  return <div>Something is {props.something}</div>;
}

I could really see a lot of value in passing other values in on the ComponentArgs. I've done a similar thing is a React-like library, where the shape of the ComponentArgs looked like this:

    let entityParams = {
      previousProps,
      previousChildren,
      previousState,
      previousContext,
      props,
      children,
      context,
      state,
      setState,
      getParams,
    };

this helped me keep things simple and defined going into each "component".

Also, check out the getParams value. It it a function that returns the current state of the object (the "entityParams"), and can be passed through to external functions such as event handlers. This kind of pattern might be really helpful in dealing with some of the things that have to be done in Async-ish components.

Another important topic that is helped greatly with avoiding the use of this is testing. It's much easier to fully test a component if it is idempotent.

This is all just my 2 cents, but this pattern has worked really well for others that I've worked with, and just thought I'd share.

[tschaub]

You’re saying we put the props in their own key on the object passed to components, so that we can pass other keys

@brainkim - yeah, that's what I had in mind.

The Simple Component example would become:

function Greeting({props: {name = "World"}}) {
  return (
    <div>Hello {name}</div>
  );
}

And deeper in the Loading Component example:

function *RandomDogApp({addEventListener, refresh}) {
  let throttle = false;
  addEventListener("click", (ev) => {
    if (ev.target.tagName === "BUTTON") {
      throttle = !throttle;
      refresh();
    }
  });

  while (true) {
    yield (
      <Fragment>
        <div>
          <button>Show me another dog.</button>
        </div>
        <RandomDogLoader throttle={throttle} />
      </Fragment>
    );
  }
}

Etc.

I've only just started looking at crank (haven't yet made the time to try it out), and you've clearly thought deeply about it, so these ideas might be misguided. But I have been left with uncertainty about a few things that I thought might not be issues with a slightly different API.

For example, I'll annotate the function below with my uncertainty:

async function *RandomDogLoader({throttle}) {
  // huh?  I wonder what I might want to do here
  // is the value of `throttle` above always the same as in the first iteration below???
  // it would be weird to see what happens if I yield an element here
  for await ({throttle} of this) {
    yield <LoadingIndicator />;
    yield <RandomDog throttle={throttle} />;
  }
}

[brainkim]

Hmmmm so I thought some more and still think the current approach is the most ergonomic. There’s something really intoxicating about the idea that a component can just be a function which takes props and returns elements, and that’s something I want to preserve. One way to frame the decision is to think of the parameter list of a component as important real estate, so much so that adding an extra level of destructuring for addEventListener or refresh, things which are available to every component anyways, would markedly decrease readability and writability.

As far as what do we do between the parameters and the loop over this, the reasons I like the current approach is that I’ve found it very helpful to have some notion of “before we start responding to props” or “the initial render.” With stateful components, there is almost always some setup logic you need to do, and having it happen outside the main props loop makes it clear that you’re doing setup logic visually. Also, if you’re not using typescript, you can do:

async function *RandomDogLoader() {
  for await (const {throttle} of this) {
    yield <LoadingIndicator />;
    yield <RandomDog throttle={throttle} />;
  }
}

and just not pass parameters, but even then I think having the props defined in the parameter list is incredibly helpful for understanding your components at a glance.

[monodop]

One thing that hasn't been mentioned yet is that you can't use arrow functions to define your components in Crank, as you don't have access to this. We should check to make sure there aren't any real drawbacks from this limitation.

Edit: Additionally, some other libraries use this in their apis. Could cause some interference if people aren't careful. I can't think of any recent examples though. Having a hard time googling for that lol.

[brainkim]

@monodop See https://github.com/bikeshaving/crank/issues/26#issuecomment-615930117 where I talk more about why I chose to use this too!

[lukejagodzinski]

I've just found out about Crank and I really like the idea. I've read whole documentation, run all examples and had some thoughts. Now, I'm reading through all the issues to see what problems people see with this library/framework so that I don't talk about something that was already mentioned. And I'm here :). When I first noticed {propName} of this I was thinking "what the hell is that". It's definitely very unintuitive thing but people will probably get used to it. However, if "current" props have to be passed in the context then I would rather change it from this to this.props, so that at first glance you know what you're dealing with. Later, you will get to know that it's a current props value. Personally I would prefer props in args to always be the newest version but I don't know internals of this library, and from what you're saying it's not as easy to implement. So just throwing in my thoughts :)

[lukejagodzinski]

Also, what happens if someone defines property for the component that is same as context function for example:

async function *Test({ addEventListener }) {
  for await({ addEventListener } of this) {
    /* ... */
  }
}

Would it override context's addEventListener function? I know that an example of addEventListener as prop is terrible but it could be whatever that is in the context :)

[monodop]

There's a couple logical leaps that I think aren't super intuitive, but the basic idea is that you're doing something like this.

const propUpdates = this;
for await (const newProps of propUpdates) {
  // yield something
}

You don't actually destructure this. Instead you destructure the props returned when there's a change, so you shouldn't have any weird conflicts between the context api and props like you described.

[lukejagodzinski]

@monodop hmmm I don't see any difference between your example and mine as it's still the same this, isn't it? But maybe first I have to refresh my knowledge about loops in async generators :) I've never had to use them in real project

[monodop]

Yeah my point is that they are the same. When you call {addEventListener} of this, it's not destructuring this. It's destructuring the props that were provided to your component. Since addEventListener is a method on this, and not a prop, you wouldn't get a conflict. And even if you did make a prop with that name, it's referring to a different thing so there's still no conflict

[lukejagodzinski]

@monodop ok just one more question to make it clear for me :D. Is it because this object contains symbol property for iterator and async iterator, so that it will be used instead of actual context object?

If so, I think it would also be good to add that to docs, to make it clearer for people who are new to (async) generators and (async) iterators :)

[tschaub]

Currently, things work kind of like this:

// in a pretend version of crank.js
const context = {
  addEventListener: () => {
    /** register a listener */
  },
  refresh: () => {
    /** do refresh stuff */
  },
  // etc.
};

context[Symbol.iterator] = function* () {
  yield {someProp: "some value"};
  // do other things
  yield {someProp: "some other value"};
};

// calling a user-supplied component
SomeComponent.call(context, {someProp: "initial value"});

and users do this

// in some application that uses crank.js
function* SomeComponent(initialProps) {
  this.addEventListener("click", () => console.log("got clicked"));
  for (const props of this) {
    yield "got props!";
  }
}

My proposal was roughly this:

// in a modified pretend version of crank.js
const context = {
  addEventListener: () => {
    /** register a listener */
  },
  refresh: () => {
    /** do refresh stuff */
  },
  props: {
    [Symbol.iterator]: function* () {
      yield {someProp: "some value"};
      // do other things
      yield {someProp: "some other value"};
    },
  },
  // etc.
};

// calling a user-supplied component
SomeComponent(context);

and users would then do this:

// in some application that uses crank.js
function* SomeComponent(context) {
  context.addEventListener("click", () => console.log("got clicked"));
  for (const props of context.props) {
    yield "got props!";
  }
}

The destructuring is optional in both cases.

[elektronik2k5]

@tschaub's proposal makes sense to me. However, it should be inspected with a bit of context, for which I was gonna open a separate issue: is there a goal of making Crank interoperable with (some) of the React ecosystem? Specifically, modern features: function components and hooks. If yes (even if that's a long term goal and not an immediate one), it severely limits our API options.

I'd appreciate your feedback @brainkim.

[ryhinchey]

@elektronik2k5 From what I’ve read there’s no desire to make react specific patterns and libraries work with Crank. IMO that’s a good thing. This should be a project to experiment without carrying around the baggage of always having to answer to how it works with the most popular component library in the world.

[brainkim]

@elektronik2k5 What @ryhinchey said. There are some nice things from React that we should take, and I think the signature of function components is one of them, but many things, especially hooks 😡 should probably be left behind.

@tschaub I get your proposed API change but don’t necessarily see the value of not using this to model contexts. See also my comments in https://github.com/bikeshaving/crank/issues/26 where I do a defense of why I think this is actually great for the use-case of contexts.

[lukejagodzinski]

I was playing around with generators and trying to find the best syntax that would make everyone happy. The best thing would be just to not use this context to get a new version of props. I have a terrible idea from the good practices perspective but it would work :P. Just posting it here:

// Create context for the component.
var ctx = { props: { firstName: "John", lastName: "Smith" } };

function *foo(props) {
    let i = 0;
    while(true) {
        i++;
        yield `${JSON.stringify(props)}: ${i}`;
    }
}
var i = foo(ctx.props);

i.next();
// {value: "{"firstName":"John","lastName":"Smith"}: 1", done: false}

i.next();
// {value: "{"firstName":"John","lastName":"Smith"}: 2", done: false}

// Now lets say someone passes a new set of props: props = { firstName: "William", lastName: "White" }
Object.assign(ctx.props, { firstName: "William", lastName: "White" });

i.next();
// {value: "{"firstName":"William","lastName":"White"}: 3", done: false}

Of course, it's a very a simple example when the props count and names are the same in all calls. In a real life, we can't be sure that someone won't add or remove props. So there should be more logic that can deal with it.

const props = { firstName: "John", lastName: "Smith" };
const nextProps = { firstName: "William", age: 30 };

const nextKeys = Object.keys(nextProps);
const keys = Object.keys(props);
for (const key of keys) {
  props[key] = nextProps[key];
  // Or if we want to use `delete` keyword which is probably bad idea:
  // if (nextProps[key] === undefined) {
  //   delete props[key];
  // } else {
  //   props[key] = nextProps[key];
  // }
}
for (const key of nextKeys) {
  props[key] = nextProps[key];
}

// without delete
// {firstName: "William", lastName: undefined, age: 30}

// with delete
// {firstName: "William", age: 30}

I know that it's bad and probably could have other downsides like reusing same reference to props which could cause problems with props comparison in "component should update". But just leaving it here for consideration.

[ndp]

I'm coming to this discussion late, but I was thinking about exactly this while walking the dog this morning (before reading all this). Coming from React (and many other frameworks):

• argument to the function could conventionally be called initialProps
• await for..of this stuff could by convention (without destructuring) be await for props of this, which to me, reads pretty nice.
• I'm not afraid of this... I think it keeps things simple and clean and you only have to learn it once. I feel like because of the scoping in Javascript, this is easily thought of as context. I like the implicit nature of it.

[brainkim]

@ndp My dog passed away last year, and I really do miss going on long walks while thinking about code. I’ve gotten the feedback that code like for await ({throttle} of this) feels like way too much syntax to throw at a person not steeped in the latest ECMAScript proposals all at once, so I’m receptive to ideas like naming the parameter initialProps and iteration values props or nextProps. The great thing about parameters and for loops is that the user can choose names for these variables as it make sense to them. I chose to use destructuring because I found it to be convenient, but other people can come up with their own naming patterns and we can all still be using the same framework.

[lukejagodzinski]

@brainkim what do you think about the solution I proposed above?

[brainkim]

@lukejagodzinski I’m not sure aliasing the props variable is the right approach, even it does seem interesting. With JSX and createElement, we essentially have to create a copy of props anyways, so then to go and have props be the same object for each update feels scary. Like what if the user mutates the props within the component, and then we update, are those mutations blown away? Interesting idea nonetheless!

[lukejagodzinski]

@brainkim yep it is scary :) so that's why I'm not like 100% sure that's a good idea. Hmmm in the first place, props shouldn't be modified. Actually, the best would be to make it immutable but with my approach it can't be.

I'm like this approach btw:

function *Test({ firstName, age }) {
  /* ... */
  for ({ firstName, age } of this.props) {
    /* ... */
  }
}

and people will get use to it when they start using generators more. Was thinking if there is a way to just make it look more like React code but probably there are too many downsides.

And actually different syntax of stateless and stateful components is something that makes sense as they both serve different purpose.

[keyvan-m-sadeghi]

@brainkim why not pass the context as the second argument?

async function *Component({ message } ,{ updates, refresh, addEventListener }) {
  for await({ message } of updates) {
    /* ... */
  }
}