pnp / pnpjs

Fluent JavaScript API for SharePoint and Microsoft Graph REST APIs
https://pnp.github.io/pnpjs/
Other
764 stars 305 forks source link

How do I deal with localstorage / sessionstorage overflow when using cache? #3160

Closed nbelyh closed 1 week ago

nbelyh commented 3 weeks ago

What version of PnPjs library you are using

4.x

Minor Version Number

4.1.0

Target environment

All

Additional environment details

I'm using pnpjs in a browser (client-side). I am using caching. https://pnp.github.io/pnpjs/queryable/behaviors/#caching

Question/Request

When the cache overflows, the PNPJS does not seem to handle that in any reasonable way, it just falls through with an error, there is no way to degrade gracefully. Chrome has 10mb local Storage limit for example.

Here is the code that puts value in the local storage, does not seem to have any checks: https://github.com/pnp/pnpjs/blob/a6611e03dccff5704b8d6f93e15b95d477d7ab26/packages/core/storage.ts#L71

If the cache is full, the above line will result in an exception, that will fail the query itself.

What could be a correct strategy to handle that? I would like the query not to fail, but I would like to clear cache, or just return an uncached result in this case. How do I approach this?

patrick-rodgers commented 3 weeks ago

Well, I'll say that in however many years this library has existed you're the first I can remember who has filled the storage. 10mb is a lot of json.

I am not sure if a check exists for "is cache full" so we could catch that error. But then what, do you think its right to fail silently as things are not cached? Said a different way - an error has occurred in what you asked the library to do - how should we handle that?

Do you think the cache is getting full with non-json data? Like are we caching images somewhere we should stop?

nbelyh commented 3 weeks ago

@patrick-rodgers Yes, in my case, there are "packed thumbnails" inside the JSON (like, data: urls). They are not that big by themselves (maybe something 20-50kb each), but the app makes quite a number of queries (hundreds), and they just keep piling up. Also, the app holds them for longer than the default 5 minutes. Plus "normal" query results. The app is graphics (svg)-intensive, so that also contributes to the things.

To be honest, I'm not sure how a library could handle that properly. Maybe some sort of callback on cache failure could do, to give the application a chance to deal with the problem?

Writing Caching behavior from scratch could be also a solution (for me), but maybe I'm just the first one to hit this, so it could be good for the library to have some strategy to deal with that?

bcameron1231 commented 1 week ago

Hi. We've chatted a bit about this and we do not believe this to be something we will be incorporating in to the library. We provide the means and recipes for basic caching scenarios. If you're interested in building your own behavior and doing a PR into the library, we would welcome that as well.

Unfortunately, there isn't much else we can do to help here. A custom behavior is the approach I would recommend.

nbelyh commented 1 week ago

@bcameron1231 thank you for the response! For now, I have worked around the issue by stripping out the image thumbnails from the queries (limiting the fields in the query), that reduced the load to some reasonable size even in the worst cases for my app.

If I decide to submit a PR for this, what approach can I take? I was thinking of extend the existing Cache with some option (callback) to call if the cache is full.

bcameron1231 commented 1 week ago

I would recommend you create a brand new custom behavior for implementing your caching scenario.

If you have questions on custom behaviors, please see some guidance. https://pnp.github.io/pnpjs/core/behaviors/ https://pnp.github.io/pnpjs/core/behavior-recipes/

and our contributing docs https://pnp.github.io/pnpjs/contributing/

nbelyh commented 1 week ago

@bcameron1231 Thank you for the info. By suggesting to create a PR did you assume adding feature to a library, or am I missing the point?

To add a feature of handling cache overflow, a modification is required for the caching module, as far as I understand? Adding some new caching won't fix that, I mean

bcameron1231 commented 1 week ago

Hi. We do not want to add any additional overflow support to the existing Caching mechanism. I am asking if you add overflow support add a new Behavior for Caching which supports it. That behavior could be added to the library and allow other developers to use it if the default Cache behavior doesn't work for their needs.

nbelyh commented 1 week ago

I see, thanks 👍

I don't think a PR to the library makes much sense this way though, could be a sample or something. There is a good rule of thimb to start fixing an issue if it happens at least three times, and probably nobody will run into this for the next five or ten years 😅

github-actions[bot] commented 4 days ago

This issue is locked for inactivity or age. If you have a related issue please open a new issue and reference this one. Closed issues are not tracked.