Add new obsidian-readwise plugin

[renehernandez]

[x] I am submitting a new Community Plugin

Repo URL

https://github.com/renehernandez/obsidian-readwise

Release Checklist

• I have tested this on macOS (I don't have access currently to a Linux or Windows machine)
• Github release contains all required files
  • main.js
  • manifest.json
• Github release name matches the exact version number specified in your manifest.json (Note: Use the exact version number, don't include a prefix v)
• The id in my manifest.json matches the id in the community-plugins.json file.
• README clearly describes the plugins purpose and provides clear usage instructions.

[lishid]

This is required in order to pull the highlights from Obsidian into your vault.
Did you mean "from Readwise"

https://github.com/renehernandez/obsidian-readwise/releases/tag/0.0.1
Seems like the release file is 1.8mb which probably means some big library got embedded. Not sure which one it is, possibly "luxon". Is there any chance moment.js is a viable replacement (since we already bundle it with the app).
Also, consider switching

https://github.com/renehernandez/obsidian-readwise/blob/03b2eca5db75e12604ce1719cb67864a5a322ad8/src/utils.ts#L4
I think a better idea is to store the token in localStorage, which is normally encrypted on disk and don't risk being picked up by backup software.

https://github.com/renehernandez/obsidian-readwise/blob/03b2eca5db75e12604ce1719cb67864a5a322ad8/src/index.ts#L90
Use the better loading mechanism in the sample

https://github.com/renehernandez/obsidian-readwise/blob/03b2eca5db75e12604ce1719cb67864a5a322ad8/src/settings.ts#L96
This doesn't serialize/deserialize well to/from JSON does it? Why not just use a unix timestamp number throughout?

https://github.com/renehernandez/obsidian-readwise/blob/03b2eca5db75e12604ce1719cb67864a5a322ad8/src/index.ts#L14
These seems unused.

[renehernandez]

@lishid Thanks for the feedback! I'll review/update accordingly

[lishid]

Yup, looks like if you get rid of the secrets mechanism, you can remove all the NodeJS dependencies which means the plugin could even work on mobile.

[renehernandez]

Seems like the release file is 1.8mb which probably means some big library got embedded. Not sure which one it is, possibly "luxon". Is there any chance moment.js is a viable replacement (since we already bundle it with the app).

@lishid Are you suggesting that I declare moment.js as devDependency in my package.json?

My javascript/typescript skills are limited 😂

[lishid]

You're already using Moment.js from https://github.com/renehernandez/obsidian-readwise/blob/81f47a129be30922247b4a082bb3a2c90505cddf/src/status.ts#L69

That is the correct usage since Obsidian bundles a copy of moment.js. You don't need to add it to your dependencies.

I'm not familiar with what you're using luxon for, but if it's just for manipulating and displaying dates then moment.js should suffice.

[lishid]

I would suggest using unix timestamps throughout your code, and making it into a moment object only when you need to manipulate or display with let obj = window.moment(timestamp)

[renehernandez]

@lishid

• Any suggestions about how to store the token using localStorage? I haven't used it before and I think I could benefit from any examples or pointers.
• Are there any other Obsidian plugin leveraging localStorage?

[lishid]

Any suggestions about how to store the token using localStorage? I haven't used it before and I think I could benefit from any examples or pointers.

MDN has some good docs: https://developer.mozilla.org/en-US/docs/Web/API/Window/localStorage#example

Are there any other Obsidian plugin leveraging localStorage?

Not that I know of other than our internal plugins.

The benefit of using localStorage is that it's not directly accessible from the file system (It's stored in an encrypted blob inside the user's appdata), and that it's shared across multiple vaults. Though sharing it cross vault may be undesirable depending on what you're trying to do.

[renehernandez]

Not that I know of other than our internal plugins.

I am not sure how easy would be (depending on how coupled/decoupled is the code), but I could benefit from having the internal plugins being open source. It would facilitate creating/following standard practices around plugin development

[lishid]

I understand. It's still fairly coupled at the moment as it's sharing some basic tooling and UI components with the app, as well as using a completely different (older) plugin loading/hooking mechanism because we haven't had time to migrate them to the new public plugin format yet.

[renehernandez]

@lishid I was reading about localStorage and I found out this alternative: https://github.com/sindresorhus/electron-store. Do you have any thoughts about it?

[renehernandez]

@lishid And sorry for all the spammy questions. My JS/TS knowledge, specially with electron, is limited 😂

[lishid]

electron-store is used for the electron main process. For Obsidian, you should treat the entire thing as a browser webpage, unless you need specific electron APIs that can't be achieved with browser APIs.

Also our mobile apps aren't electron based, so if you use electron APIs, your plugin would be unable to run on our mobile clients.

[renehernandez]

@lishid As part of the localStorage implementation, I am thinking it would be nice to delete the token if the plugin is removed. I looked into the obsidian api definitions, but I don't see any hook for deletion?

Is this supported?

FYI: LocalStorage PR: renehernandez/obsidian-readwise#12

[lishid]

Ah you're right, there is no API for uninstall. The closest thing you can do is to do that in the onunload but I'm not fully sure of the implications.

[renehernandez]

@lishid All the issues you raised have been resolved and released as part of version 0.0.2.

Now the main.js file only weights ~250Kb.

Thanks for all the help!

[lishid]

https://github.com/renehernandez/obsidian-readwise/blob/72736bea12807a4e2918812a2c45145c42ce6a21/src/tokenManager.ts#L6
I don't think this cast is necessary. localStorage is of type Storage according to my lib.dom.d.ts.

https://github.com/renehernandez/obsidian-readwise/blob/72736bea12807a4e2918812a2c45145c42ce6a21/src/index.ts#L128
localStorage returns null if the value doesn't exist, and you might error out on .length here.

https://github.com/renehernandez/obsidian-readwise/blob/72736bea12807a4e2918812a2c45145c42ce6a21/src/settings.ts#L8
It's just a small typing problem but this doesn't actually return an instance of ObsidianReadwiseSettings with the proper prototypes, but rather just a plain object. I would recommend making your settings class into an interface.

https://github.com/renehernandez/obsidian-readwise/blob/72736bea12807a4e2918812a2c45145c42ce6a21/src/fileDoc.ts#L6
This won't work on mobile because NodeJS apis aren't available. Maybe you can find a replacement?

[renehernandez]

https://github.com/renehernandez/obsidian-readwise/blob/72736bea12807a4e2918812a2c45145c42ce6a21/src/fileDoc.ts#L6
This won't work on mobile because NodeJS apis aren't available. Maybe you can find a replacement?

I have raised an issue about a new join method that should be part of the FileSystemAdapter public interface at obsidianmd/obsidian-api#12.

In the meantime, I'll write my join implementation to remove the usage of the path API from NodeJS.

Also, I don't see an easy way of detecting the platform (win32 vs others) without resorting to use the process module, which would defeat the purpose of removing the path module

[renehernandez]

@lishid Based on the comment above, I am ok to release the first version of the plugin as Desktop only and address it once the Obsidian API supports a join method

[lishid]

Obsidian always use / internally, and there is a normalizePath that converts any kind of paths into obsidian-normalized paths that can be used with the Vault API. Hope that helps!

[renehernandez]

Obsidian always use / internally, and there is a normalizePath that converts any kind of paths into obsidian-normalized paths that can be used with the Vault API.

I missed that one!

[renehernandez]

@lishid I have implemented the suggested fixes and I have released a new plugin version 0.0.3. Let me know if you have any other feedback

[lishid]

I think we're at a point where this can be merged - just a couple of more small nits that aren't functionally important, but more code style related.

https://github.com/renehernandez/obsidian-readwise/blob/214ceefd8e9bae6c7dd09939548f6eb8f05ab4d7/src/index.ts#L117
Don't rely on the concrete class FileSystemAdapter here, especially since you aren't using anything specific to that concrete class. Use Adapter (the interface) instead.

https://github.com/renehernandez/obsidian-readwise/blob/214ceefd8e9bae6c7dd09939548f6eb8f05ab4d7/src/tokenManager.ts#L9
I would suggest making this return just string | null where null indicate not found, instead of [boolean, string].

[renehernandez]

@lishid Awesome. I added 2 new issues to track their implementation for a future 0.1.0 release.

Thanks a lot for your feedback and guidance!