search

jamiebrynes7 / obsidian-todoist-plugin

Materialize Todoist tasks in Obsidian notes
https://jamiebrynes7.github.io/obsidian-todoist-plugin/
MIT License
912 stars 70 forks source link

Fix memory leaks #328

Open iLiftALot opened 2 months ago

iLiftALot commented 2 months ago

Memory Leak Error Message

This is a solution for the following error indicating potential memory leaks:

Error: Plugin "todoist-sync-plugin" is not passing Component in renderMarkdown.
This is needed to avoid memory leaks when embedded contents register global event handlers.
    at t.render (app.js:1:1298057)
    at t.renderMarkdown (app.js:1:1297838)
    at eval (plugin:todoist-sync-plugin:13:5673)
    at ev (plugin:todoist-sync-plugin:1:4888)
    at Array.map (<anonymous>)
    at eval (plugin:todoist-sync-plugin:4:3833)
    at sv (plugin:todoist-sync-plugin:4:1281)

I definitely notice a performance increase after the adjustment. This was happening because the component life cycle was not being properly managed. It was solved by creating a componentRef.

NOTE: It seems like this slightly changes the formatting of the final render, but nothing drastic. Personally, I enjoy the change, it appears more aesthetic in my opinion, so I opted to not make any changes.

Explanation of Changes Made

  1. Component Management

A componentRef is created to manage the lifecycle of the Component. The componentRef is initialized if it's null, ensuring a single instance is used.

  1. Lifecycle Handling

The useEffect hook manages the rendering and cleanup of the component. On cleanup (return () => { ... }), the renderChild is removed from the component, and the component is set to null to avoid memory leaks.

  1. Context Use

The RenderChildContext is used correctly to get the renderChild context. The renderChild is added to the Component which is then passed to the MarkdownRenderer.renderMarkdown method.

jamiebrynes7 commented 2 months ago

Hey @iLiftALot, thanks for the contribution. I'm a little bit confused though. In between the last public release (v1.13.0) and the current master, I've actually rewritten the UI rendering completely. This may go some way to explaining the aesthetic difference and performance change you observed.

The point though, is that I thought I did fix this issue during that rewrite. Did you observe the error on a build from master before your fix? Or have you only observed it when using the public release?

For completeness, in the previous releases, I was fetching the component ref from a Svelte context when rendering Markdown (https://github.com/jamiebrynes7/obsidian-todoist-plugin/blob/1.13.0/plugin/src/components/MarkdownRenderer.svelte#L13), but the context was never initialised properly leading to the error log you saw.

iLiftALot commented 2 months ago

Hey @iLiftALot, thanks for the contribution. I'm a little bit confused though. In between the last public release (v1.13.0) and the current master, I've actually rewritten the UI rendering completely. This may go some way to explaining the aesthetic difference and performance change you observed.

The point though, is that I thought I did fix this issue during that rewrite. Did you observe the error on a build from master before your fix? Or have you only observed it when using the public release?

For completeness, in the previous releases, I was fetching the component ref from a Svelte context when rendering Markdown (https://github.com/jamiebrynes7/obsidian-todoist-plugin/blob/1.13.0/plugin/src/components/MarkdownRenderer.svelte#L13), but the context was never initialised properly leading to the error log you saw.

No problem @jamiebrynes7

I noticed the error in the developer console while using both the latest version of the public release (v1.13.0) as well as the latest master release.

Notice that a Component instance is created if it doesn't already exist. The renderChild context is then attached to the Component instance, which is passed within MarkdownRenderer.renderMarkdown, ensuring proper lifecycle management.

Additionally, I just submitted another update which adds a cleanup for any potential resources that are left over.

  • RIP

These additions solved the error on my end.

By no means am I an expert; I guarantee you are far ahead of me in terms of knowledge regarding the use of TypeScript/JavaScript, especially React. The purpose of this pull request is more to bring it to your attention, get your mind going on why this resolved the error, and potentially lead to an improved version.

I've studied Python for ~7-8 months before switching over to JavaScript/TypeScript over the past 2 months, so when I noticed the error, I looked at it as a great learning opportunity. Programming has evolved into an unhealthy obsession rather than a hobby 😂.

I also love the plugin and use it every day within my daily and weekly notes. Up until now I've never submitted a pull request.

I'm currently working on implementing additional features such as task deletion, pulling/displaying comments, etc. There are ways to implement these things according to the Todoist documentation, I just need to find my way around your plugin code 😅

My schedule is pretty open until the Fall, so let me know if you have more questions about this - I'd be glad to help in any way possible.