-
Notifications
You must be signed in to change notification settings - Fork 3
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
[WIP] fix: remove event listeners on unmount #8
base: master
Are you sure you want to change the base?
Conversation
mermaid.initialize(config); | ||
return mermaid.run(options); | ||
onUnmountRef.current = mermaid.initialize(config); | ||
mermaid.run(options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please return promise from mermaid.run
const config = omit(props, [ 'querySelector', 'nodes', 'onError' ]); | ||
const options = pick(props, [ 'querySelector', 'nodes' ]); | ||
const config = omit(props, ['querySelector', 'nodes', 'onError']); | ||
const options = pick(props, ['querySelector', 'nodes']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unexpected diff
@@ -64,9 +64,22 @@ async function next(): Promise<void> { | |||
|
|||
const { zoom } = mermaid.mermaidAPI.getConfig() as InitConfig; | |||
|
|||
let onUnmount = [() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable assignrd only once, why let
?
}); | ||
|
||
return null; | ||
} | ||
|
||
export function useMermaid() { | ||
const [ mermaid, setMermaid ] = useState<Parameters<Callback>[0] | null>(null); | ||
const onUnmountRef = useRef<any>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot to add ";" at the end of the operation. What about linters in the current project?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the same question
let onUnmount = [() => { | ||
document.removeEventListener('click', zoomBehavior) | ||
}] | ||
|
||
document.removeEventListener('click', zoomBehavior); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This string too
document.addEventListener('click', (e) => { | ||
const cb = zoomBehavior(e) | ||
if(cb){ | ||
onUnmount.push(cb) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what is happening here.
Why do you collect the zoomBehavior
handler on every click
in the document?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -64,9 +64,22 @@ async function next(): Promise<void> { | |||
|
|||
const { zoom } = mermaid.mermaidAPI.getConfig() as InitConfig; | |||
|
|||
let onUnmount = [() => { | |||
document.removeEventListener('click', zoomBehavior) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This string doesn't make sense because nobody calls
document.addEventListener('click', zoomBehavior)
No description provided.