Update findDocumentOrShadowRoot to return the root node instead of throwing unnecessarily
#4427
Conversation
…wing unnecessarily
🦋 Changeset detectedLatest commit: a37ecb6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Thank you @ben10code . This has very much been a problem for me as well in production and I was just about to tackle it so I really appreciate it. And the second commit really helped make this easier to review so thanks for that as well.
The general approach looks fine to me. I'm going to ask others on the Slate team for a quick review and hopefully we can land this quickly.
Also if you could add a changeset that would be great (helps us automate the release notes). You can run yarn changeset or in your branch and it will help you generate one. Thanks!
packages/slate-react/src/components/android/android-editable.tsx
Outdated
Show resolved
Hide resolved
| throw new Error( | ||
| `Unable to find DocumentOrShadowRoot for editor element: ${el}` | ||
| ) | ||
| return undefined |
There was a problem hiding this comment.
Looking back at https://github.com/ianstormtaylor/slate/pull/3749/files , I think this may just suppress things, rather than restoring support for window/window.document. Perhaps better would be:
| return undefined | |
| return ReactEditor.getWindow(editor).document |
And then we wouldn't need the change to support | undefined throughout? Unless I'm missing something (e.g. if window wasn't actually working previously). And perhaps throw if that is also not defined?
There was a problem hiding this comment.
| throw new Error( | ||
| `Unable to find DocumentOrShadowRoot for editor element: ${el}` | ||
| ) | ||
| return ReactEditor.getWindow(editor).document |
There was a problem hiding this comment.
I think this could be simplified to:
| return ReactEditor.getWindow(editor).document | |
| return el.ownerDocument |
Though to be honest, I think the entire method could be re-written, the logic is hard to follow currently.
Something like:
findDocumentOrShadowRoot(editor: ReactEditor): Document | ShadowRoot {
const el = ReactEditor.toDOMNode(editor, editor)
const root = el.getRootNode()
if ((root instanceof Document || root instanceof ShadowRoot) && root.getSelection != null) {
return root
}
return el.ownerDocument
}There was a problem hiding this comment.
Updated, indeed much cleaner! Also tested affected examples to ensure they still work :)
ben10code
changed the title
findDocumentOrShadowRoot to return undefined instead of throwing unnecessarilyfindDocumentOrShadowRoot to return the root node instead of throwing unnecessarily
…throwing unnecessarily (ianstormtaylor#4427) * Update `findDocumentOrShadowRoot` to return undefined instead of throwing unnecessarily * Small refactoring to improve the diff for reviewers * Add changeset for patch * Update new-trainers-peel.md * Resolve PR comments * Revert undefined checks, return window.document and update changeset * Simplify findDocumentOrShadowRoot based on PR feedback * Re-run CI Thanks everyone for your review and thanks @ben10code for your first contribution!
Description
In Slate v0.62, a new util method
getDocumentOrShadowRootwas introduced. This latter returns theShadowRootorDocumentnode in which the editor is rendered but throws when not found.Unable to find DocumentOrShadowRoot for editor element: [object HTMLDivElement]This seems to be causing issues for consumers who render the editor within React portals. The issue arises at unmount time, because
getRootNodereturns the now-detached portal node.Version
0.61.3does not have this issue and seems to be the go-to for most right now.Issue
Fixes: #4267
Example
In this first example, the editor is rendered inside a modal that uses React portal. When first opening the modal, it works fine - but closing the modal (unmounting) and reopening will crash because
rootisn't an instance ofDocumentorShadowRootanymore.https://codesandbox.io/s/slate-reproduction-reakit-portal-forked-yeykl?file=/index.js:902-907
In this second example, the portal is created using
createPortaland then appended to the document later. In the initial render, the root node is not theDocumentand throws. (Example by JackRobards)https://codesandbox.io/s/slate-reproduction-reakit-portal-fk002
Context
This pull request is straightforward and shouldn't introduce unwanted behaviours. Instead of throwing when the root isn't an instance of
DocumentorShadowRoot, I simply returnundefined. In places where the root node is used, I simply check whether it exists before performing operations.I've been trying to gather context surrounding the reasons why we throw, but couldn't find anything. Feel free to share if you have context 🙏 !
Checks
yarn test.yarn lint. (Fix errors withyarn fix.)yarn start.)yarn changeset add.)