Small bug (and fix) in codesandbox TodoMVC about crank HOT 13 CLOSED

Just as a remark: There are also subtle logical bugs in the TodoMVC demo. The problem is that the component TodoItem does not handle prop value changes properly.
It will not be a problem in this simple TododMVC demo, but yet the implementation of the component TodoItem is not really correct:

• let title = todo.title - this mutable title variable is used e.g. directly in the blur event handler. For example in the following case this will not work as desired: Component TodoItem will be mounted with a certain todo prop => then the todo prop will be changed (without unmounting) => then the double-click happens => then the "blur" happens (without any key presses in between) => problem: That todo.edit custom event will be dispatch with the wrong title property.

• As title is also used directly in the JSX output, again for example in case that the prop todo changes without any key presses in between, after a double-click the output may show the wrong title.

• The ESC_KEY is not handled properly - should behave like a dismiss not like an apply.

Component Mainand Footer:

• Both the Main component function and the Footer component functions are effectful - shouldn't they be generator functions? Otherwise those event handlers will be registered on each refresh.

from crank.

Oddly I can’t reproduce it in Safari, and scarily this seems to be an error caused somewhere between the typescript generator output and Chrome. Will investigate further. In the meantime, possible related issues:

microsoft/TypeScript#9372
redux-saga/redux-saga#703

Thank you for the report!

from crank.

@kyeotic

Why is the removal of TodoItem not resulting in the event listeners being removed?

You’re right the event listener wasn’t being removed in time. However there is a more fundamental problem, which is that because event dispatch is synchronous, a developer might inadvertently cause a sync cascade of updates between parent and child:

function Child(this: Context) {
	this.dispatchEvent(new Event("test", {bubbles: true}));
	return <span>child</span>;
}

function* Parent(this: Context) {
	this.addEventListener("test", () => {
		try {
			this.refresh();
			done();
		} catch (err) {
			done(err);
		}
	});

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

renderer.render(<Parent />, document.body); // Generator is already executing

The solutions were:

1. Let the error happen. This was tempting because it felt like this would make the error discoverable, and also it was the least amount of work, but I didn’t like it because as we see, this kind of error could show up in weird cross-browser ways.
2. Silently fail. We could just disable refreshes while the component is updating. Inevitably, the developer will figure out that their attempts to refresh while the component was still rendering are futile and move dispatchEvent calls into an event callback, or maybe just a setTimeout callback if they’re lazy. We may want to log a warning in the future but I’m not sure how warnings work yet.
3. Enqueue another refresh synchronously. This was tempting but it could cause its own problems like infinite loops where the child keeps refreshing the parent until we reach the stack limit.
4. Enqueue another refresh asynchronously. One design principle I’ve been trying to adhere to with the library is to make sync things synchronous and async things asynchronous, so this option was out of the question.

My preferences for solutions were 2, 1, 3, 4. I chose 2 and maybe we’ll warn when people write code which attempts to synchronously step through a generator multiple times.

from crank.

@brainkim That makes sense, I wasn't thinking about the sync loop. I agree that #2 is good as long as their is a warning, with the possibility of turning it into an error via configuration somehow. Errors have the advantages of stack traces, which can greatly simplify debugging in large component trees.

from crank.

Fixed in 0.1.2. Thanks for the bug report it was very helpful!

from crank.

@brainkim ok it makes sense to choose option 2 as these event listeners won't be executed anyway but browser is just trying to do so. Thanks for clarification :). Also it would be good to check in specs of generators what should be the correct behavior. Maybe Chrome just does it wrong? From experience I would say that Safari is doing something wrong :P but maybe this time it's different story :)

Also about errors/warnings management. I actually don't have anything against those console warnings. But in the ideal world it could be just supported by the code editor (eslint plugin maybe?) that just tells you as you write that something is wrong. There could also be a bundler plugin that checks such stuff and informs you about problems. However, correct me if I'm wrong those React errors does not leak into production, there are only seen in dev env, right?
IMHO, the best option would be eslint plugin, like react-hooks plugin for eslint.

Ok now I understand the difference. Yep I don't think those onevent's should be removed/nullified. They will be removed anyway by the garbage collector.

Thanks!

from crank.

Edited to add V8 version:
JavaScript: V8 8.3.110.4

from crank.

Checking in my investigation into the bug before going to take a shower to think about solutions:

1. Why does this happen in Chrome and not Safari (my main browser)? When focused elements are removed from the DOM, Chrome will fire a blur event. So what’s happening in the specific TodoMVC example, the keydown event is called, which triggers a todo.destroy event. This causes the app to rerender and remove the TodoItem from the page, which in turn triggers the blur event in Chrome, which triggers a second todo.destroy event, all synchronously. Here’s a minimal reproduction of the problem:
   https://codesandbox.io/s/lucid-butterfly-v0jqg?file=/src/index.js

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

const ENTER_KEY = 13;
function* Input() {
  this.addEventListener("keydown", ev => {
    if (ev.keyCode === ENTER_KEY) {
      console.log("KEYDOWN");
      this.dispatchEvent(new Event("bug.destroy", { bubbles: true }));
    }
  });

  this.addEventListener("blur", ev => {
    console.log("BLUR");
    this.dispatchEvent(new Event("bug.destroy", { bubbles: true }));
  });

  while (true) {
    yield <input class="edit" value="Hello?" />;
  }
}

function* App() {
  let show = true;
  this.addEventListener("bug.destroy", ev => {
    show = false;
    this.refresh();
  });

  while (true) {
    yield <div>{show && <Input />}</div>;
  }
}

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

2. Why is this a problem? Intrinsic elements are backed by a generator to keep track of its state. When an element commits, the generator is stepped through, props are compared, and children are added and removed. The problem occurs when the element attempts to remove a child of a DOM node and this triggers a blur event, which triggers another update. Generators cannot iterate themselves while executing. This is true of any spec-compliant generator implementation:

let a;
function *b() {
  let i = 0;
  while (true) {
    if (a) {
      a.next();
    }
    yield i++;
  }
}
const c = b();
c.next(); // {value: 0, done: false}
c.next(); // {value: 1, done: false}
a = c;
c.next();
// TypeError: Generator is executing

It’s very seldom that a generator can reference or trigger its own iterator while executing, but this is something which can happen in Crank and which we should guard against.

Solutions: TK

from crank.

This causes the app to rerender and remove the TodoItem from the page, which in turn triggers the blur event in Chrome, which triggers a second todo.destroy event, all synchronously.

Why is the removal of TodoItem not resulting in the event listeners being removed? Is it just an order-of-operations issue in Crank?

from crank.

@brainkim Hey, I would like to clarify. Is Crank removing event listeners before next refresh operation? You said that refreshes are synchronous. So the blur even should never be called, no matter what browser it is. Right?

And another thing about 4 options that you presented. Which one did you choose?

from crank.

@lukejagodzinski I chose option 2, fail silently. We may end up adding warnings to the library indicating that you called refresh on a synchronously iterating component, but I think failing silently was the best choice because as we saw in the issue, the situation can differ according to subtle differences in the ways different browsers handle events, and it can happen if, for instance, the user synchronously dispatches an event from the child of a generator component.

I have to think a little about what sort of warnings Crank should emit, because I think the situation in React where the console yelled at you for every little thing like rendering an array whose members aren’t keyed was really annoying, and I don’t want to replicate that development experience. I’d much prefer it if things you did wrong were actually just discoverable as you write and test your app rather than having the console yell at you.

As far as event handlers go, Crank removes event handlers when nodes are unmounted in the case of using the this.addEventListener API, but not yet when you’re using the onevent props APIs. We may remove onevent props from removed nodes, I just want to make sure there aren’t any actual ramifications to nulling out onevent callbacks, and I’m not sure it’s completely necessary, given that most modern browsers don’t really care if you null out onevent props. If you think we should clear onevent props when nodes are removed from the DOM it’ll probably be a couple extra lines of code. But again, most of the danger of having event callbacks fire one last time before nodes are removed has been eliminated with the fix above. Let me know what you think.

from crank.

let title = todo.title - this mutable title variable is used e.g. directly in the blur event handler. For example in the following case this will not work as desired: Component TodoItem will be mounted with a certain todo prop => then the todo prop will be changed (without unmounting) => then the double-click happens => then the "blur" happens (without any key presses in between) => problem: That todo.edit custom event will be dispatch with the wrong title property.

As title is also used directly in the JSX output, again for example in case that the prop todo changes without any key presses in between, after a double-click the output may show the wrong title.

What are the exact user actions which trigger this bug? I’m having trouble understanding. If you’re saying that the todo item related to the currently edited todo is being changed as the user is editing it, it’s not exactly clear what the correct behavior should be, and todomvc leaves it underspecified. My intuition is that even if you had some external events affecting the todos, the most pleasant ux would be to not change the currently edited todo, and let the user decide what to do.

Meanwhile, don’t you appreciate how you’re able to reason about local changes relative to parent updates 😃

The ESC_KEY is not handled properly - should behave like a dismiss not like an apply.

My bad I’ll make that fix eventually if someone doesn’t get to it first.

Both the Main component function and the Footer component functions are effectful - shouldn't they be generator functions? Otherwise those event handlers will be registered on each refresh.

We clear event listeners when the component is a sync function component as a convenience. It doesn’t really matter for performance in todomvc but you may want to use a generator function if the component is frequently updated as an optimization.

from crank.

We clear event listeners when the component is a sync function component as a convenience.

Ah okay, I see, thanks - stupid me. That may be the reason why you always talk about "basic"/"sync"/"simple" components and not about "stateless" components ;-) ... But frankly, I'm not really sure whether I see a big win in having access to this in those "basic"/"sync"/"simple" components as it allows to do confusing things and IMHO everything that you can do with "basic"/"sync"/"simple" components which is not pure/stateless/effectless feels like an anti-pattern to me (just to save two lines of code) - but that's just MHO, of course.
[Edit - 11 hours later]: Fun fact: I just found out that I myself have actually used this in the basic components of my TodoMVC variant #71 (comment) ... anyway, I still think it is an anti-pattern :-)

What are the exact user actions which trigger this bug?

Maybe that's more a matter of what we expect a really correct implemented component should look like. IMHO, a component should ALWAYS behave in reasonable and determinate way independent what happens in the outside world.
Regarding this mutable title variable: This will only be updated on input and keydown and on keydown only a title = title.trim() will be performed.
Maybe it's easier to say that title = ev.target.value.trim() should be used in the keydown and blur event listeners. And in that for loop, the variable title should be updated in case that we are not in "edit mode" and title differs from todo.title.

PS: I personally would leave the edit mode automatically in case the values of todo.id or todo.title change, this may confuse the user a bit in that moment but at least no data would be changed in a undesired manner - but I guess nobody's doing it that way in those dozens of TodoMVC implementations ;-), so anyway...

from crank.