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

[WIP] fix: remove event listeners on unmount #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[WIP] fix: remove event listeners on unmount #8

Show file tree
Hide file tree
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
19 changes: 11 additions & 8 deletions src/react/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { InitConfig, RunOptions, ZoomOptions } from '../types';
import { useEffect, useState, useCallback } from 'react';
import { useEffect, useState, useCallback, useRef } from 'react';

export type RuntimeOptions = {
onError?: (error: any) => void
Expand Down Expand Up @@ -31,24 +31,25 @@ function omit<O extends Record<string, any>, P extends string, R extends {

export function MermaidRuntime(props: InitConfig & RunOptions & RuntimeOptions) {
const renderMermaid = useMermaid();
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']);
Copy link
Member

Choose a reason for hiding this comment

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

Unexpected diff


useEffect(() => {
renderMermaid(config, options).catch(props.onError || (() => {}));
renderMermaid(config, options).catch(props.onError || (() => { }));
});

return null;
}

export function useMermaid() {
const [ mermaid, setMermaid ] = useState<Parameters<Callback>[0] | null>(null);
const onUnmountRef = useRef<any>()
Copy link
Contributor

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?

Copy link
Author

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

const [mermaid, setMermaid] = useState<Parameters<Callback>[0] | null>(null);
const render = useCallback(async (config: InitConfig, options?: RunOptions) => {
if (mermaid) {
mermaid.initialize(config);
return mermaid.run(options);
onUnmountRef.current = mermaid.initialize(config);
mermaid.run(options);
Copy link
Member

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

}
}, [ mermaid ]);
}, [mermaid]);

useEffect(() => {
(window.mermaidJsonp = window.mermaidJsonp || []).push(setMermaid);
Expand All @@ -58,6 +59,8 @@ export function useMermaid() {
if (index > -1) {
window.mermaidJsonp.splice(index, 1);
}

onUnmountRef.current?.()
};
}, []);

Expand Down
15 changes: 14 additions & 1 deletion src/runtime/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,22 @@ async function next(): Promise<void> {

const { zoom } = mermaid.mermaidAPI.getConfig() as InitConfig;

let onUnmount = [() => {
Copy link
Member

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?

document.removeEventListener('click', zoomBehavior)
Copy link
Contributor

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)

}]

document.removeEventListener('click', zoomBehavior);
Copy link
Contributor

Choose a reason for hiding this comment

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

This string too

if (zoom) {
document.addEventListener('click', zoomBehavior);
document.addEventListener('click', (e) => {
const cb = zoomBehavior(e)
if(cb){
onUnmount.push(cb)
Copy link
Contributor

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?

Copy link
Author

Choose a reason for hiding this comment

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

https://github.com/diplodoc-platform/mermaid-extension/pull/8/files#diff-37537d8d0d9ef0302c0dced9490f77b09137d976bbc9d8d85a8f718b4bf40439

}
});
}

return () => {
onUnmount.map(cb => cb())
}
},
render: mermaid.render,
Expand Down
5 changes: 5 additions & 0 deletions src/runtime/zoom.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,11 @@ export function zoomBehavior(event: Event) {
};

document.addEventListener('mousedown', onOuterClick, true);

return () => {
disableZoom()
document.removeEventListener('mousedown', onOuterClick, true);
}
}

function getZoomOptions(element: HTMLElement): ZoomOptions {
Expand Down