Small bug (and fix) in codesandbox TodoMVC #70
Comments
|
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 Thank you for the report! |
|
Edited to add V8 version: |
|
Checking in my investigation into the bug before going to take a shower to think about solutions:
/** @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);
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 executingIt’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 |
Why is the removal of TodoItem not resulting in the event listeners being removed? Is it just an order-of-operations issue in Crank? |
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 executingThe solutions were:
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. |
|
@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. |
|
Fixed in 0.1.2. Thanks for the bug report it was very helpful! |
|
@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? |
|
@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 |
|
@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? Ok now I understand the difference. Yep I don't think those Thanks! |
|
Just as a remark: There are also subtle logical bugs in the TodoMVC demo. The problem is that the component
Component
|
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 😃
My bad I’ll make that fix eventually if someone doesn’t get to it first.
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. |
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
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. PS: I personally would leave the edit mode automatically in case the values of |
TL;DR: The crank.js example at https://codesandbox.io/s/crank-todomvc-k6s0xIt sometimes causes an exception around the re-rendering and dispatch of two
"todo.destroy"events from theTodoItem, breaking the app. The immediate fix I found is instead of dispatching the"todo.destroy"event from thekeydownhandler, just callev.target.blur().While using the crank.js example at https://codesandbox.io/s/crank-todomvc-k6s0x when I edit a todo, delete the all the characters from the title, and the press the
enterorescapekey, the following exception is thrown in the console:Browser info:
It looks like the code is dispatching two
"todo.destroy"events, one whenenter/escapeis pressed (through thekeydownhandler) and also, whenactiveis set tofalseand theTodoItemis re-rended (or something else) causes theblurevent to be triggered on the input, thus dispatching a second"todo.destroy"event. Not sure why that blows up so spectacularly though.The fix I found is instead of dispatching the
"todo.destroy"event from thekeydownhandler, just callev.target.blur(). There may be a better error to be showing or some way to catch it.The text was updated successfully, but these errors were encountered: