Improve StorageDataSource

[doup]

⚠️ Breaking Changes

This is a breaking change, the fix is easy though:

• Now it throws NotFoundError instead of returning null
• Rename LocalStorageDataSource to StorageDataSource
• Add the desired Storage implementation as first parameter, usually localStorage or sessionStorage.

From:

new LocalStorageDataSource()

To:

new StorageDataSource(localStorage)

Change List

• Rename LocalStorageDataSource to StorageDataSource, as this can be used with any Storage implementation (e.g. LocalStorage, SessionStorage…)
• Add SafeStorage helper (which fallbacks to an in-memory implementation) to handle incognito issues, see: https://michalzalecki.com/why-using-localStorage-directly-is-a-bad-idea/
• Handle null getItem value and throw NotFoundError
• Handle setItem errors and throw FailedError
• Handle values/keys length mismatch and throw InvalidArgumentError

[lucianosantana]

I like the idea. But, is this safe-switch responsibility of the DataSource or is it responsibility of the Repository? Who's the one to decide that In Memory is an acceptable fallback?

[doup]

I don't know which is the correct approach here. My guess was that it's safer to do it implicit, so we've less bugs. Otherwise we need to be conscious and be explicit about these issues (which is a rare consideration, as these are edge cases).

In the context in which these issues arise, I think it's OK to default to an in-memory Storage. And, in case we want to do something fancier we can always add a repository which checks if local/session storage isSupported. This doesn't close that option.

[lucianosantana]

Maybe we could get input from the mobile team. They might have similar cases. What do you think @franmontiel ?

[franmontiel]

On mobile I think we do not have any particular case to highlight but here is my opinion.

Storage is the framework-specific implementation and extending it to solve a framework-specific problem in my opinion is perfectly fine.

DataSourceStorage can either use the normal Storage or SafeStorage version so that change is decoupled from the DataSource itself.

With this approach, you can use another generic repository on top of DataSourceStorage. If you create a repository this won't be possible.

From an architectural point of view, I think it is not problematic because even repositories responsibility is to be the "business logic" for the data layer (so it makes sense that any decision on where to fetch data belong to them) in this particular case I see this as a workaround for certain undesired platform behavior.

One last thing that I would improve is to make super clear in the documentation that the SafeStorage does not guarantee that the data is really stored on local storage.

[doup]

Just to clarify:

• Storage is a web platform interface, both "local storage" & "session storage" implement that interface.
• SafeStorage is a wrapper for any Storage, and itself implements Storage. I've added this to take care of some edge cases that happen on incognito or when the user has restricted permissions. What it basically does is to fallback to an in-memory Storage in case of initialisation error.
• Previous LocalStorageDataSource is now StorageDataSource, so it accepts any kind of Storage.

Decouple/couple SafeStorage from StorageDataSource?

• Decoupled. Unhandled errors on edge cases. Current situation.
• Coupled. Swallows errors from edge cases, defaults to an in-memory implementation.

In either case we can provide any instance that implements Storage and we can move the edge-case handling to a repository in case we need something fancier.

---

I was thinking, what about adding a required second parameter safeMode: boolean to StorageDataSource? This way the user needs to take a conscious decision to either enable the wrapper or not. If we leave the wrapping as a responsibility of the user she might not be aware of the need to use SafeStorage.

// User responsibility
new StorageDataSource(new SafeStorage(window.localstorage)); // /w SafeStorage
new StorageDataSource(window.localstorage); // raw LocalStorage

// With required `safeMode`
new StorageDataSource(window.localstorage, true); // /w SafeStorage
new StorageDataSource(window.localstorage, false); // raw LocalStorage

[lucianosantana]

@doup I think your suggestion of explicitly enabling the "safe mode" depending on a flag parameter solves the issue I've raised (the repository still can choose the behavior).

Another minor suggestion: instead of naming the parameter safeMode: boolean we could have it explicit/verbose allowInMemoryFallback: boolean

[doup]

I haven't changed the parameter name as it's more nuanced than just "fallback to in-memory". Instead I've set a docblock that explains what the param does. What do you think?