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.

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

Wrong type of container returned from useCodeMirror #474

Closed
yifanwww opened this issue Mar 20, 2023 · 5 comments · Fixed by #639
Closed

Wrong type of container returned from useCodeMirror #474

yifanwww opened this issue Mar 20, 2023 · 5 comments · Fixed by #639

Comments

@yifanwww
Copy link
Contributor

In the document/README, it says the type of container returned from useCodeMirror is:

container: HTMLDivElement | null | undefined;

However, in the latest version (v4.19.9), the actual type that is exported from the npm package is:

container: HTMLDivElement | undefined;

I checked the source code, and I believe the type in document is right.


The source code

// https://github.com/uiwjs/react-codemirror/blob/6968faf74121748c629ab9487d5172beec9de42e/core/src/useCodeMirror.ts#L150

useEffect(() => setContainer(props.container!), [props.container]);
  1. In the first render, the value of props.container is actually null which is the initial value of the ref editor in ReactCodeMirror.
    This change triggers a second render, because the new value null is different from the current value undefined.
  2. Then in the second render, it can get the HTMLDivElement and pass it to container again, triggering a third render.
  3. In the third render, the container finally becomes HTMLDivElement.

During the whole process, the container value is actually undefined -> null -> HTMLDivElement.

@jaywcjlove
Copy link
Member

@yifanwww thx! Upgrade v4.19.10

jaywcjlove added a commit that referenced this issue Mar 25, 2023
@jaywcjlove
Copy link
Member

@yifanwww I haven't solved the problem. I tried to fix the code, but it caused the useCodeMirror hook to fail to update the container, resulting in a rendering failure of the interface.

@yifanwww
Copy link
Contributor Author

The only thing we can do to fix the issue, I think, is just changing the type.

We cannot change the logic, as the first time the container changes from undefined to null is needed. It can only get the HTMLDivElement instance in the second render.

-    container: editor.current,
+    container: editor.current ? editor.current : undefined,

This change will disable the second render, so in useCodeMirror the container will never get the HTMLDivElement instance.

I don't know how to improve it, I didn't find a better way...

@yifanwww
Copy link
Contributor Author

Oh maybe we can follow the doc https://github.com/uiwjs/react-codemirror#support-hook

useEffect(() => {
  if (editor.current) {
    setContainer(editor.current);
  }
}, [editor.current]);

In this useEffect, it can get the HTMLDivElement instance in the first render. Because editor is a ref, ref is updated before useEffect.

@jaywcjlove
Copy link
Member

I tested it and there are still issues even with multiple state updates within the useCodeMirror hook.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants