Passing context as a function argument

[migueloller]

The current API uses this to provide a component context that has useful methods related to the component. Has it been considered to pass the context as an argument to the function. Maybe something like this?

function Counter(props, context) {
  let count = 0

  context.addEventListener('click', () => {
    count++

    context.refresh()
  })

  return <button>Count: {count}</button>
}

This might feel more familiars to users that use arrow functions (although you couldn't use arrow functions for generator functions anyway), and could allow easier static typing by not having to worry about typing this. It also goes nicely with the general direction the ecosystem has been leaning towards; being as functional as possible, by not accessing implicit values like this.

[skclusive]

i guess we can already achieve this expected behaviour using default parameters.

function useCounter(context) {
  const count = { value: 0 };
  context.addEventListener("click", () => {
    count.value++;
    context.refresh();
  });
  return count;
}

function* Counter(props, context = this) {
  const count = useCounter(context);
  for (;;) {
    yield <button>Count: {count.value}</button>;
  }
}

renderer.render(<Counter />, document.body);

working sample https://codesandbox.io/s/shy-breeze-t90oe?file=/src/index.js:130-496

[brochington]

While creating a "React without dom diffing" library, I've found it VERY helpful to not use a this bind at all, and instead pass all things into the function. FWIW I would recommend trying out something like:

function Counter({state, context}: CrankArgs): Element {
..
}

CrankArgs can also contain things like previousProps, or any functions (like refresh).

You can check out my code here: https://github.com/brochington/declarity

I'm really liking the return to more straightforward JS in Crank. Very exited to see where it's heading.

[brainkim]

I definitely thought about passing context in as an argument rather than a function, so you’re in good company. I decided against it for the following reasons:

1. Generator functions (currently) have no arrow function equivalent. Insofar as you must necessarily use the old function syntax for generators, I see no harm in using this.
2. I know these be fighting words, but personally speaking, I think top-level arrow functions with const are worse than just using the function keyword. It’s literally more characters to type when your body is a block:

const fn = () => {
// vs
function fn() {

3. Not passing the context as an argument means we can add arguments to components later on. This means that in the future, if we want to add more arguments to components we can. It’s strictly more flexible.
4. Components should not ever be called by the user! Components can be sync functions, async functions, sync generators or async generators. This means that the return value of components is very broad: it can be a value, a promise which fulfills to a value, or a generator/async generator which yields/returns values. Handling all these cases is the majority of what the Crank core library does. Passing contexts as an argument rather than this implicitly encourages beginners to try calling components directly, or attempt to share contexts between components. I strongly discourage this sort of practice (though I can’t stop you), and I know that you can use Function.prototype.call/apply to bind the context of a function, but this initial friction is really useful to prevent this sort of experimentation, at least until you know a little bit of JavaScript.

As far as why we can’t do as @brochington suggests, the biggest problem is TypeScript. TypeScript types function components using the first parameter. So if we tried to put all the context stuff in a nested object, we’d lose strongly typed components.

I know the this stuff might look weird at first, but these were some of the reasons why I chose it.

[migueloller]

This all makes sense. As to a practical reason as to why one might want to materialize a context, and this isn't necessarily solved by my OP, what's the plan for testing? Will there be some utilities provided by Crank to test components? If so, I guess these utilities could provide the context, either as a parameter or via this.

[skclusive]

Generator functions (currently) have no arrow function equivalent. Insofar as you must necessarily use the old function syntax for generators, I see no harm in using this.

yes. there is no harm using this. but it might confuse new comers. at least i was, when using jQuery event handlers where this would be the event.currentTarget.

Also this is provided implicitly, without typescript it would be hard to reason about it.

And as mentioned in above comment, it forces to have testing wrapper library to mock and invoke the component for testing.

Not passing the context as an argument means we can add arguments to components later on. This means that in the future, if we want to add more arguments to components we can. It’s strictly more flexible.

As of now props and context are available objects for components. what would be the compelling argument in future? even if there would be one, could it be next to context? function Component(props, context, future)

If the ambition is use JS features and runtime as much as possible, treating components as just functions (sync, async, generator and async-generator) returns jsx elements of respective return types, with known arguments props, context, ...rest, would be great.

[skclusive]

Also regarding context sharing, would you not encourage following hooks pattern? unlike react, hooks definitions are not re-created every time. i see that as biggest advantages of Crank.js.

function useCounter(context, value = 0) {
  const count = { value };
  context.addEventListener("click", () => {
    count.value++;
    context.refresh();
  });
  return count;
}

function* Counter(props) {
  const count = useCounter(this, props.count);
  for (;;) {
    yield <button>Count: {count.value}</button>;
  }
}

function useStorageCounter(context, value = 0) {
  const stored = localStorage.getItem("--count--");
  const count = useCounter(context, (stored && parseInt(stored, 10)) || value);
  context.addEventListener("click", () => {
    localStorage.setItem("--count--", count.value);
  });
  return count;
}

function* StorageCounter(props) {
  const count = useStorageCounter(this, props.count);
  for (;;) {
    yield <button>Count: {count.value}</button>;
  }
}

https://codesandbox.io/s/flamboyant-brahmagupta-1dqjs?file=/src/index.js:755-910

[brainkim]

@migueloller

what's the plan for testing? Will there be some utilities provided by Crank to test components? If so, I guess these utilities could provide the context, either as a parameter or via this.

This is a great question and probably merits its own issue. We could imagine creating a mock context, and testing components that way.

@skclusive

yes. there is no harm using this. but it might confuse new comers. at least i was, when using jQuery event handlers where this would be the event.currentTarget.

Crank was designed from the ground-up to be friendly to beginners, and if you tell them that this is special and has these sorts of methods for components, they’ll probably just believe you and not think too hard about how that worked. You can elide the info about how this works for another day, and such knowledge wouldn’t be necessary to get productive with Crank.

Also regarding context sharing, would you not encourage following hooks pattern? unlike react, hooks definitions are not re-created every time. i see that as biggest advantages of Crank.js.

Yeah you can definitely share contexts with helper functions like that! What I’m explicitly discouraging is sharing contexts between functions which were meant to be independent components; this hurts my brain and I don’t want to see it done.

[BrandonNoad]

If you decide to stick with using this, you may want to have a convention where you assign this to a variable at the start of the function/generator: const context = this;. AngularJS had a similar convention for their controllerAs syntax (https://github.com/johnpapa/angular-styleguide/blob/master/a1/README.md#style-y032).

For generator components, you may also want to name the parameter initialProps in the docs, which may help with the "bug where the component kept yielding the initial message even though a new message was passed in via props".

/** @jsx createElement */
import { createElement, Fragment } from "@bikeshaving/crank";
import { renderer } from "@bikeshaving/crank/dom";

function* Clicker(initialProps) {
  const context = this;

  let count = 0;

  const handleClick = (e) => {
    count += 1;
    context.refresh();
  };

  for (const { msg } of context) {
    yield (
      <Fragment>
        <p>{msg} {count}</p>
        <button onclick={handleClick}>Click Me!</button>
      </Fragment>
    );
  }
}

renderer.render(<Clicker msg="Count:" />, document.body);

[brainkim]

@BrandonNoad I’ve used ctx cuz that’s what I used to do in canvas programming as in:

const ctx = this;

For generator components, you may also want to name the parameter initialProps in the docs, which may help with the "bug where the component kept yielding the initial message even though a new message was passed in via props".

That was mostly a pedagogical tool to help people understand that contexts were an iterable of props; I don’t really imagine it will be a bug people encounter very often.

[BrandonNoad]

I don’t really imagine it will be a bug people encounter very often.

Once people are familiar with how Crank works, then I agree that is true. But you may want to make the docs as beginner-friendly as possible. I remember being a bit confused by it the first time I read the docs. By naming the parameter initialProps in the docs, I think it would help the readers understand how it works more quickly.

[kyeotic]

I know these be fighting words, but personally speaking, I think top-level arrow functions with const are worse than just using the function keyword. It’s literally more characters to type when your body is a block:

+1 to the whole thing, but especially this part

[wmadden]

I agree it would be good to avoid using this in general. I discourage it in my teams wherever possible because it's impossible to tell by reading the code what it will be at runtime (without the explicit typing TypeScript provides) and it's possible for code outside to set it to the wrong value by accident.

Obviously these problems aren't possible when Crank is setting this to the context for each call, but I still consider this an antipattern in JavaScript.

Not passing the context as an argument means we can add arguments to components later on. This means that in the future, if we want to add more arguments to components we can. It’s strictly more flexible.

I didn't understand this point. You can add more arguments to a function's signature but you certainly can't add more this references. How is using this more flexible? The API will lock context to the this reference forever. @brochington suggested named arguments above which would be more flexible than using either this or ordered arguments, although it would break the React-style function(props) signature

[kyeotic]

I didn't understand this point. You can add more arguments to a function's signature but you certainly can't add more this references. How is using this more flexible? The API will lock context to the this reference forever. @brochington suggested named arguments above which would be more flexible than using either this or ordered arguments, although it would break the React-style function(props) signature

If Crank adds more functionality to this, like say this.newMethod() the signature of the Component function doesn't change at all. You can just start using the new feature once you update. Its not about adding it at runtime, its about adding it to the API of Crank.

[eferte]

Hello, sorry but my english is not very good, I really like this new framework, it's so refreshing !

I was playing with it and the "this" keyword is a bit dangerous for me too. By the way, I don't know if it's possible, but what do you think of separating the iterator part of the context in something called "renderIterator" like that :

function* TodoItem({refresh, renderIterator}, {todo}) {

  let title = todo.title;
 
  const handleLabelDblClick = () => {
   	...
	refresh();
  }
  ...
  for ({ todo } of renderIterator) {
    yield (
      <li class={classes.join(" ")}>
        <div class="view">
          <input onclick={handleToggleClick} class="toggle" type="checkbox" />
          <label ondblclick={handleLabelDblClick}>{todo.title}</label>
        </div>
      </li>
    );
  }
}

Here "this" disappear and context can be destructured (or not) if we like it.

It's just an idea, not sure it's a good idea :-)

Anyway, again, this framework is very interesting, I love it !

[wmadden]

If Crank adds more functionality to this, like say this.newMethod() the signature of the Component function doesn't change at all. You can just start using the new feature once you update. Its not about adding it at runtime, its about adding it to the API of Crank.

@kyeotic if this is a reference to the context object, and you mean adding methods to the context object, then it doesn't matter how you're passing the context object to the function the API won't have to change.

The API has to change if you want to pass a reference to a second object to the component, and in that case named arguments would mean the least amount of change.

[kyeotic]

Yea, I'm not trying to say the context-as-param model doesn't work, I was just trying to clarify something you said you didn't understand.

Briankim already gave a good comparison of the two models

[wmadden]

As far as why we can’t do as @brochington suggests, the biggest problem is TypeScript. TypeScript types function components using the first parameter. So if we tried to put all the context stuff in a nested object, we’d lose strongly typed components.

@brainkim you can of course just use the second parameter, like so (copying @brochington's example):

function Counter(props: { a: number }, context: Context): Element {
  // ..
}

// props correctly inferred
renderer.render(<Counter a={1} />, document.body);

(And you could use named arguments so it's easy to extend the API later)

function Counter(props: { a: number }, other: { context: Context }): Element { }

[brainkim]

@wmadden

I agree it would be good to avoid using this in general. I discourage it in my teams wherever possible because it's impossible to tell by reading the code what it will be at runtime (without the explicit typing TypeScript provides) and it's possible for code outside to set it to the wrong value by accident.

This recommendation probably makes sense for your team and when working on application code, but we can play by a different set of rules when writing framework code, as in code which establishes conventions and uses inversion of control. I repeat, only Crank should be invoking components; most of the complexity of the Crank core is built around calling and handling the many different possible return types of components. Crank developers do not have to worry about this being set to the wrong value, nor is it “impossible” to tell by reading the code what it will be at runtime, because we’ve established by convention that components are always called with a this set to a Crank context. Also, by your logic, we should be worried about the props passed into components too because “it’s possible for code outside to set it to the wrong value by accident”. What makes positional parameters less prone to being set incorrectly than this?

Obviously these problems aren't possible when Crank is setting this to the context for each call, but I still consider this an antipattern in JavaScript.

If you want to call something an anti-pattern, you should be able to back it up with concrete instances where the pattern causes bugs or otherwise degrades developer experience. I have yet to see a reason provided by anyone besides “I don’t like this.”

I didn't understand this point. You can add more arguments to a function's signature but you certainly can't add more this references. How is using this more flexible? The API will lock context to the this reference forever.

Here’s one way to think about it. Imagine we did as you say and pass the context as the second parameter of components. This would be strictly less-flexible than using this because suddenly we have to consider the context positionally. There is an expectation by JavaScript programmers that you order positional arguments in order from most frequently used to least frequently used (think options parameters being last), and when APIs get this wrong you’re forced to do weird API calls like JSON.stringify(value, null, 2), where we’re forced to pass null as the second argument even though we never use or even really know what it does.

Concretely, think about the following API change. What if we decided that rather than passing children in with props, we decided to pass them in as a positional argument? This is mainly just a demonstration of a possible API change to components rather than a serious proposal. How would we order them if we also passed contexts in positionally? function Component(props, ctx, children) looks wrong to me, but function Component(props, children, ctx) would be a breaking change.

This is what I mean when I say using this is more flexible. Contexts probably aren’t going away no matter how much this library changes, and there will always be a context created for each component because of how component trees work. At the same time, not every component uses the context; specifically, sync and async function components almost never have a reason to reference the component context. So we have a thing which is always passed in but not always used. Sound familiar? this is also always present and doesn’t have to be used, and is therefore great for modeling a component’s context.

Finally, one thing that React folks have trouble with is distinguishing React function components from regular functions. This is important for things like hot reloaders, which parse your code and make sure exported components are reloaded when files change. This couldn’t be easier with Crank, and especially with TypeScript, because components are distinguished from regular functions because they’re passed the context as the this keyword rather than as a parameter. This convention helps us distinguish components from helper functions which are passed contexts but don’t return or yield children. And trying to identify components by return value would be more difficult, insofar as like I said, components can return virtually anything and still be a component. Again, it’s about establishing conventions, and this is both unusual enough to distinguish components from helper functions, and perfectly models what a context is, which is an ever-present object which provides methods to help the component refresh itself or access new props.

I get that you’re allergic to this, and you’re in good company when you say you’re skeptical of using it, but I’d like to argue that Crank contexts and components are a perfect use-case for using the this value of functions dynamically. As Dan Abramov states in a tweet thread that I can’t find right now, components are neither strictly functions nor classes, and by passing in a this we can capture this half-class/half-function quality of components, by giving components access to methods without the mismatches of inheritance or instantiation. I bet you five years out, we’ll see other important JavaScript libraries use this in this way, and we’ll even come up with a name for this design pattern, like “the injected this controller pattern” or something.

[brainkim]

@eferte You’re in good company! This has been suggested several times already and I’ve considered it. Ultimately, I decided against it because there’s something beautiful about sync function components being a function which takes props which returns JSX elements; It is a part of the React component API I would like to keep. Also, TypeScript types function components based on their first parameter. I don’t like a lot of the React APIs and there’s a lot we can change, but I don’t think the function signature for components is one of them. The less we change about the original React API for function components, the easier it will be for people to migrate, as that’s a pretty big part of the React API.

[monodop]

If you use the this keyword, you're locking users into using function for components, and arrow functions for everything else. You've addressed any of my concern around removing the ability for lambda function components. Like you said, generator components have to be functions anyway, so I don't really have any real objections other than personal preference. But what about the reverse?

For example, taking your Timer example on codesandbox

function* Timer(this: Context) {
  let seconds = 0;
  const interval = setInterval(() => {
    seconds++;
    this.refresh();
  }, 1000);
  try {
    while (true) {
      yield <div>Seconds: {seconds}</div>;
    }
  } finally {
    clearInterval(interval);
  }
}

I have to use an arrow function for my interval. I can't write the code like this:

function* Timer(this: Context) {
  let seconds = 0;
  function onTick() {
    seconds++;
    this.refresh();
  }
  const interval = setInterval(onTick, 1000);
  try {
    while (true) {
      yield <div>Seconds: {seconds}</div>;
    }
  } finally {
    clearInterval(interval);
  }
}

If I try to do that, I get an error that this doesn't have a method called refresh() since I've lost access to the context. I'm sure some users will find this behavior to be confusing, especially if they're not using TypeScript.

I bet it could also cause problems if someone wants to create a generator or component inside their component, but realistically I doubt that will happen often enough to change the api for it. It's something I've been meaning to experiment with, and maybe there's some interesting patterns that could come from it.

You might find this to be a weak argument, but I wonder if there's anything we can do to reduce any unnecessary headaches?

[wmadden]

If you want to call something an anti-pattern, you should be able to back it up with concrete instances where the pattern causes bugs or otherwise degrades developer experience. I have yet to see a reason provided by anyone besides “I don’t like this.”

@brainkim this gets unset a lot by accident. Here's a simple example:

function* MyComponent() {
  let i = 0;

  setTimeout(function increment() {
    i += 1;
    this.refresh();
  }, 1000);

  while (true) {
    yield <div>{i}</div>;
  }
}

[EDIT: Oops - I see @monodop already pointed this out]

[wmadden]

Concretely, think about the following API change. What if we decided that rather than passing children in with props, we decided to pass them in as a positional argument? This is mainly just a demonstration of a possible API change to components rather than a serious proposal. How would we order them if we also passed contexts in positionally? function Component(props, ctx, children) looks wrong to me, but function Component(props, children, ctx) would be a breaking change.

You can use named parameters to avoid concerns about the order of parameters, as I noted above. The only constraint you have is that the first argument needs to be props in order for TypeScript to infer prop types correctly. So in your example where children is moved from props to another parameter:

function Counter(props, { context, children }) { }

Finally, one thing that React folks have trouble with is distinguishing React function components from regular functions. This is important for things like hot reloaders, which parse your code and make sure exported components are reloaded when files change.

Well I have no opinion on marking components as different to regular functions. It sounds unimportant to me, but I'll defer to your expertise there.

... and by passing in a this we can capture this half-class/half-function quality of components, by giving components access to methods without the mismatches of inheritance or instantiation

I think you're making this pattern sound like it has some mystical benefits that it doesn't really. I think what Axel was saying is that functions don't have state, and classes do. Components are functions which have state which is managed by the framework, not the language, so they're kinda like functions and kinda like classes. You can expose that state to the component using this, or a parameter, or a class, or even hooks' voodoo.

To recap the constraints of this problem, setting aside anyone's preferences:

• TypeScript needs the first parameter of a component function to be props in order to infer prop types for use in JSX
• React established the pattern of (props) => JSX which is worth preserving for lots of reasons
• Hot reloaders have difficulty detecting which functions are components and not without a unique signature

You could keep both camps happy with a signature that incorporates both approaches and satisfies those constraints:

type Component<Props extends { [name: string]: any }> = 
  (this: Context, props: Props, other: { context: Context }) => Element;

One final note: nobody since the orginal post has mentioned the huge functional programming community. Accepting all the inputs to the function as parameters would make the API much more acceptable in those circles.

[brainkim]

Closing as duplicate of #13