getAlby / lightning-browser-extension

The Bitcoin Lightning Browser Extension that brings deep Lightning & Nostr integration to the web. Wallet interface to multiple lightning nodes and key signer for Nostr, Liquid and onchain use.
https://getalby.com/#extension
MIT License
538 stars 194 forks source link

[Feature] Add SWR library to Alby extension #1923

Open rolznz opened 1 year ago

rolznz commented 1 year ago

Feature description

It looks like there is some custom code in lib/api and lib/cache to get account info, but it is only used for a single call and it is a custom implementation of SWR(https://github.com/vercel/swr) however with is very limited and will have to be refactored to be more general use for the other API calls.

It's best to look at the SWR project, but immediate benefits are features such as automatic retries, displaying cached data while a newer version of the data loads (removing "flicker" when switching tabs and having to wait for data to re-load), automatic debouncing, optional polling, and many more.

Introducing SWR will make the user experience better and simplify development.

Describe the solution

Add the SWR package https://github.com/vercel/swr and eventually migrate all possible API calls to use SWR when fetching data.

Describe alternatives

There are other libraries available such as https://github.com/TanStack/query

Additional context

Originally from https://github.com/getAlby/lightning-browser-extension/pull/1892#issuecomment-1366346743

Are you working on this?

None

escapedcat commented 1 year ago

Would be nice to point at (some) places inside the codebase where this could be applied to make this more actionable.

Some of our issues might not come from "real" API calls but from "getting data from background-script/brower-storage". You know what I mean?

rolznz commented 1 year ago

Would be nice to point at (some) places inside the codebase where this could be applied to make this more actionable.

Some of our issues might not come from "real" API calls but from "getting data from background-script/brower-storage". You know what I mean?

You provide your own fetcher(s) to SWR so I think it should be able to be used for any sort of request where there is expected to be a noticeable delay in fetching the data.

That is a good point though about providing examples. There are at least two so far:

I will add more to this comment when I see them.

lujakob commented 1 year ago

@rolznz I played around a little while testing how this could be implemented. Turns out not that easy (for me at least). This swr is a hook and I run into a lot of "Error: Invalid hook call. Hooks can only be called inside of the body of a function component." I have to say I'm fairly new to React thought. I got it working in the AccountContext, however the exported function fetchAccountInfo is being called outside of the component a lot. So again I run into "Error: Invalid hook call. Hooks can only be called inside of the body of a function component."

I will try a little more at the weekend. Maybe I can figure something out.

rolznz commented 1 year ago

@lujakob do you mean you got it working in AccountProvider, right? yes, it seems like it would need a big refactor.

I haven't looked too deep into the code yet. I think the existing fetchAccountInfo method should not take any parameters and should just be a SWR mutate() function if the account ID isn't changed (to trigger a re-fetch) which could be exposed by the accountProvider along with the actual data (both come from the SWR hook). To switch accounts, we should just use the existing setAccountId() method, which would also trigger a re-fetch because the fetch URL of the useSWR would depend on the current account ID. Quite a few of the fetchAccountInfo() calls may no longer be needed.

I think we should also declare a list of fetchers somewhere for fetching from different places (like an http request, or a background script, or browser storage).

Note: This task probably requires a lot of planning and I am not sure what the priority is to get it done (there aren't many requirements for this functionality yet), but in my opinion it would be nice to consider if have we have more usages in the future.

lujakob commented 1 year ago

@rolznz I was interested in this issue and started debugging and testing. I got it working at some point, that's why i share it here. I'm not sure if this is how it was intended however. Feel free go give me any suggestions on how to modify or improve. In case it doesn't help at all, I'm not bothered by discarding the whole PR in favour of other implementations.

The swr is a hook and needs to be called in every rendering turn. This kind of irritates me a little and lead to the implementation I suggest. I am fairly new to React (working on Angular for many years) though and don't have much of an idea of best practices. But as I said, if it doesn't make sense let's discard it, I learned a lot anyways.

rolznz commented 1 year ago

@lujakob regarding the hook running on every render, it returns memoized data so it won't cause multiple re-renders unless the data actually changes. See https://swr.vercel.app/docs/advanced/performance#deep-comparison