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

fix: initially set editor object only on non-ssr environments #4864

Closed
wants to merge 1 commit into from
Closed

fix: initially set editor object only on non-ssr environments #4864

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions packages/react/src/useEditor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ import {
import { Editor } from './Editor.js'

export const useEditor = (options: Partial<EditorOptions> = {}, deps: DependencyList = []) => {
const editorRef = useRef<Editor | null>(null)
// if window is undefined (SSR), return the editor instance
// otherwise we can start with a null value
const editorRef = useRef<Editor | null>(typeof window === 'undefined' ? null : new Editor(options))
const [, forceUpdate] = useState({})

const {
Expand Down Expand Up @@ -99,7 +101,9 @@ export const useEditor = (options: Partial<EditorOptions> = {}, deps: Dependency
useEffect(() => {
let isMounted = true

editorRef.current = new Editor(options)
if (!editorRef.current) {
Copy link
Contributor

@C-Hess C-Hess Feb 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take a look at #4579 if you get the chance. That pull request uses memo instead of a ref and also eliminates the double initialization that also causes issues (build failure). Best not to create the editor instance in an effect, but a memo instead; that way it gets created first render and after any deps array changes. The above PR also fixes the memory leak with the existing ref-based hook. Not a big deal to me either way if you want to just merge this PR in and abandon/close mine, but I would definitely recommend using memo. I am curious though if the types will be updated in a breaking change later, for those that extend the hook using Typescript or have ESLint rules for unnecessary null checks.

Also heads up, this is also similar to this draft PR https://github.com/ueberdosis/tiptap/pull/4858/files by @svenadlung , which has the same issue of double editor initialization.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls 🙏 @bdbch @svenadlung

editorRef.current = new Editor(options)
}

editorRef.current.on('transaction', () => {
requestAnimationFrame(() => {
Expand Down
Loading