Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Check if getBoundingClientRect exist before trying to call bind #5226

Merged
merged 2 commits into from
Dec 19, 2022
Merged

Check if getBoundingClientRect exist before trying to call bind #5226

merged 2 commits into from
Dec 19, 2022

Conversation

laufeyrut
Copy link
Contributor

Description
The current implementation of defaultScrollSelectionIntoView makes tests run by node fail.

/home/circleci/repo/node_modules/slate-react/dist/index.js:4937
    leafEl.getBoundingClientRect = domRange.getBoundingClientRect.bind(domRange);
                                                                  ^

TypeError: Cannot read properties of undefined (reading 'bind')
    at scrollSelectionIntoView (/home/circleci/repo/node_modules/slate-react/src/components/editable.tsx:1698:67)
    at setDomSelection (/home/circleci/repo/node_modules/slate-react/src/components/editable.tsx:368:9)
    at /home/circleci/repo/node_modules/slate-react/src/components/editable.tsx:376:25
    at commitHookEffectListMount (/home/circleci/repo/node_modules/react-dom/cjs/react-dom.development.js:20573:26)
    at commitLifeCycles (/home/circleci/repo/node_modules/react-dom/cjs/react-dom.development.js:20634:11)
    at commitLayoutEffects (/home/circleci/repo/node_modules/react-dom/cjs/react-dom.development.js:23426:7)
    at HTMLUnknownElement.callCallback (/home/circleci/repo/node_modules/react-dom/cjs/react-dom.development.js:3945:14)
    at HTMLUnknownElement.callTheUserObjectsOperation (/home/circleci/repo/node_modules/jsdom/lib/jsdom/living/generated/EventListener.js:26:30)
    at innerInvokeEventListeners (/home/circleci/repo/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:338:25)
    at invokeEventListeners (/home/circleci/repo/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:274:3)
    at HTMLUnknownElementImpl._dispatch (/home/circleci/repo/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:221:9)
    at HTMLUnknownElementImpl.dispatchEvent (/home/circleci/repo/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:94:17)
    at HTMLUnknownElement.dispatchEvent (/home/circleci/repo/node_modules/jsdom/lib/jsdom/living/generated/EventTarget.js:231:34)
    at Object.invokeGuardedCallbackDev (/home/circleci/repo/node_modules/react-dom/cjs/react-dom.development.js:3994:16)
    at invokeGuardedCallback (/home/circleci/repo/node_modules/react-dom/cjs/react-dom.development.js:4056:31)
    at commitRootImpl (/home/circleci/repo/node_modules/react-dom/cjs/react-dom.development.js:23151:9)
    at unstable_runWithPriority (/home/circleci/repo/node_modules/react-dom/node_modules/scheduler/cjs/scheduler.development.js:468:12)
    at runWithPriority$1 (/home/circleci/repo/node_modules/react-dom/cjs/react-dom.development.js:11276:10)
    at commitRoot (/home/circleci/repo/node_modules/react-dom/cjs/react-dom.development.js:22990:3)
    at performSyncWorkOnRoot (/home/circleci/repo/node_modules/react-dom/cjs/react-dom.development.js:22329:3)
    at /home/circleci/repo/node_modules/react-dom/cjs/react-dom.development.js:11327:26
    at unstable_runWithPriority (/home/circleci/repo/node_modules/react-dom/node_modules/scheduler/cjs/scheduler.development.js:468:12)
    at runWithPriority$1 (/home/circleci/repo/node_modules/react-dom/cjs/react-dom.development.js:11276:10)
    at flushSyncCallbackQueueImpl (/home/circleci/repo/node_modules/react-dom/cjs/react-dom.development.js:11322:9)
    at flushSyncCallbackQueue (/home/circleci/repo/node_modules/react-dom/cjs/react-dom.development.js:11309:3)
    at Object.unstable_batchedUpdates (/home/circleci/repo/node_modules/react-dom/cjs/react-dom.development.js:22387:7)
    at Object.onChange (/home/circleci/repo/node_modules/slate-react/src/plugin/with-react.ts:326:14)
    at /home/circleci/repo/node_modules/slate/src/create-editor.ts:99:18
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

Exited with code exit status 1

By checking if domRange.getBoundingClientRect exists before going into this logic we don't need to mock or send in a custom scrollSelectionIntoView function into the Editable component

Checks

  • The new code matches the existing patterns and styles.
  • The tests pass with yarn test.
  • The linter passes with yarn lint. (Fix errors with yarn fix.)
  • The relevant examples still work. (Run examples with yarn start.)
  • You've added a changeset if changing functionality. (Add one with yarn changeset add.)

@laufeyrut
Stop trying to call bind on undifined when getBoundingClientRect is n…
caa2e2e 
…ot defined. This is happening when testing slate with react testing library
@changeset-bot
Copy link

changeset-bot bot commented Dec 16, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: b2d08d9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
slate-react Minor

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

@laufeyrut laufeyrut changed the title Check if getBoundingClientRect exist before trying to call bind on it. Check if getBoundingClientRect exist before trying to call bind Dec 16, 2022
@dylans dylans merged commit 0141f68 into ianstormtaylor:main Dec 19, 2022
@github-actions github-actions bot mentioned this pull request Dec 19, 2022
Merged
@wfortin
Copy link

wfortin commented Dec 19, 2022

Running in the same issue, @laufeyrut did you find a way to workaround this in jest until this gets deployed?

@wfortin
Copy link

wfortin commented Dec 19, 2022

I was able to work around this for now by adding scrollSelectionIntoView={() => {}}

@github-actions github-actions bot mentioned this pull request Dec 20, 2022
Open
@laufeyrut
Copy link
Contributor Author

I was able to work around this for now by adding scrollSelectionIntoView={() => {}}

Yes, this is the only workaround I found, mocking this in the test was too much work.
Hopefully this will be released soon 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dylans dylans dylans approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@laufeyrut @wfortin @dylans