Support React v16 when editor is focused after removed from DOM #1409
Conversation
|
Interesting - Seems like another option would be to release the timeout in That said, it looks like the main reason we added this invariant was to improve type strictness, and having an early return does the same thing effectively of validating the type before continuing execution. I'm inclined to accept this, since I can't think of a big difference in effect between cleaning up the timeout on unmount and not throwing when it isn't cleaned up - but this way is more convenient for the engineer. Adding this to my queue for merging - thanks @mattkrick ! |
There was a problem hiding this comment.
@flarnie is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Maybe it would be cleaner to return if !editorNode then keep the invariant? Then we'll notice more explicitly if that's no longer true rather then it silently failing. |
|
Oh - I like that! Will revise this and re-rebase/etc. tomorrow. :) |
|
i'm not sure i understand the change but as long as the function terminates without an error with the editorNode doesn't exist i'm happy! |
**what is the change?:** The problem - we currently throw if '_focus' receives an invalid type of editorNode. But in some cases the editorNode is null, and it's pretty convenient/harmless to fail silently there. So the proposed change was to fail silently if the editorNode was an invalid type. However, it's a bit stricter to fail only if the editorNode is undefined or null, which is the case this is meant to address. If we get some other type of thing as the editorNode somehow, then we still throw. **why make this change?:** To be slightly stricter and still solve the problem. **test plan:** Didn't really test it! :D **issue:** No issue open afaik
There was a problem hiding this comment.
@flarnie is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| if (!(editorNode instanceof HTMLElement)) { | ||
| if (!editorNode) { | ||
| // once in a while people call 'focus' in a setTimeout, and the node has | ||
| // been deleted, so it can be null in that case. | ||
| return; | ||
| } | ||
|
|
||
| const scrollParent = Style.getScrollParent(editorNode); |
There was a problem hiding this comment.
correct me if I'm wrong, but Style.getScrollParent has 1 param & that is node, so we don't want to call it unless we have an HTMLElement, right? It doesn't matter either way (code below), but if we were writing a unit test for this, then it'd be REALLY hard to reach that invariant...
getScrollParent: function getScrollParent(node) {
if (!node) {
return null;
}
...
There was a problem hiding this comment.
@flarnie has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@flarnie has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
|
Summary: **Summary** - calling `Style.getScrollParent(editorNode)` with `null` will result in an uncaught error - `Ref#focus()` is often called within a `setTimeout` (even in some example IIRC). `Ref` doesn't get GCd, but the DOM node it points to may no longer exist. Since React v16 is so blazing fast, it's possible that we're trying to focus something that no longer exists. - Without this bit of code, we'd have to call `findDOMNode` on the ref before calling `focus`. That's not great since putting `findDOMNode` is discouraged in application code. Closes facebookarchive/draft-js#1409 Differential Revision: D5976879 fbshipit-source-id: f8f63525fdb6e76a111ea72edae1a620505f3e93
Summary: **Summary** - calling `Style.getScrollParent(editorNode)` with `null` will result in an uncaught error - `Ref#focus()` is often called within a `setTimeout` (even in some example IIRC). `Ref` doesn't get GCd, but the DOM node it points to may no longer exist. Since React v16 is so blazing fast, it's possible that we're trying to focus something that no longer exists. - Without this bit of code, we'd have to call `findDOMNode` on the ref before calling `focus`. That's not great since putting `findDOMNode` is discouraged in application code. Closes facebookarchive/draft-js#1409 Differential Revision: D5976879 fbshipit-source-id: f8f63525fdb6e76a111ea72edae1a620505f3e93
Summary: **Summary** - calling `Style.getScrollParent(editorNode)` with `null` will result in an uncaught error - `Ref#focus()` is often called within a `setTimeout` (even in some example IIRC). `Ref` doesn't get GCd, but the DOM node it points to may no longer exist. Since React v16 is so blazing fast, it's possible that we're trying to focus something that no longer exists. - Without this bit of code, we'd have to call `findDOMNode` on the ref before calling `focus`. That's not great since putting `findDOMNode` is discouraged in application code. Closes facebookarchive/draft-js#1409 Differential Revision: D5976879 fbshipit-source-id: f8f63525fdb6e76a111ea72edae1a620505f3e93
Summary
Style.getScrollParent(editorNode)withnullwill result in an uncaught errorRef#focus()is often called within asetTimeout(even in some example IIRC).Refdoesn't get GCd, but the DOM node it points to may no longer exist. Since React v16 is so blazing fast, it's possible that we're trying to focus something that no longer exists.findDOMNodeon the ref before callingfocus. That's not great since puttingfindDOMNodeis discouraged in application code.