dscheerens / ngx-webstorage-service

Module for Angular that provides service wrappers for the Web Storage API
MIT License
65 stars 11 forks source link

Angular universal support #5

Closed henniebester closed 6 years ago

henniebester commented 6 years ago

Just stumbled upon a issue which took me a while to get my head around. When using this implementation in Angular Universion the isStorageAvailable is never fired successfully because the global token 'localStorage' is not defined by default. I managed to get around this by adding global['localStorage'] = null; to my express startup code. But maybe consider adding a extra check around the area that passes in localStores and sessionStorage to the isStorageAvailable function.

if (typeof localStorage !== 'undefined') ...

dscheerens commented 6 years ago

Ah, haven't thought of the case in which the module runs within Angular Universal. Thanks for bringing this up 👍

I will try to make patch release later today to fix the issue.

dscheerens commented 6 years ago

Should be fixed in version 3.1.1 (just released)

henniebester commented 6 years ago

Tx 👍

gorkemyontem commented 5 years ago

Hi @dscheerens, I think InMemoryStorage doesn't work properly in Angular Universal environment. Since LocalStorage and SessionStorage are like one shared service, you can set a value from one component and get it from another one. However, I couldn't do the same with InMemoryStorage in server side.

Not sure but if might be related to creating a new instance of InMemoryStorageService everytime in here https://github.com/dscheerens/ngx-webstorage-service/commit/64ef276dd9a42d76cc5d778dbb330b349dab8f9d

dscheerens commented 5 years ago

I am not sure if I understand your problem correctly, but SessionStorage and LocalStorage are not sharing their storage:

localStorage.setItem('test', 'hello!');
console.log(localStorage.getItem('test')); // prints: hello!
console.log(sessionStorage.getItem('test')); // prints: null

This behavior is intentionally replicated in the session/local storage service factories. When these factories detect that the webstorage API is not available they will return a new InMemoryStorageService instance, instead of sharing the same instance.

gorkemyontem commented 5 years ago

Hi @dscheerens, Sorry for not being clear enough. I'm talking about a completely different thing.

You can do this right: /page1 localStorage.setItem('test', 'hello!');

/page2 console.log(localStorage.getItem('test')); // prints: hello!

Everything works great on the client side. However, on the server side with Angular Universal

/page1 if( isPlatformServer(platformId)){ localStorage.setItem('test', 'hello!'); } this doesn't work because there is no LocalStorage on the server side. So InMemoryStorageService comes in. Setted to in memory

However, /page2 if( isPlatformServer(platformId)){ console.log(localStorage.getItem('test')); // prints: null } We cannot get the data from memory. I think memory is deleted or a new instance starts with every refresh. On the server side, I mean NodeJs, InMemoryStorage could be a global variable and the memory could be more persistent.

I have created a service for this purpose. Please check it out. https://gist.github.com/gorkemyontem/2e5dbfac8158a355968638eadf4441f6 It's very simple.

Thanks!

dscheerens commented 5 years ago

Ah, I see what you mean.

So I haven't actually used Angular Universal myself, but from what I understand it will (re-)render the page for every request that comes in. If that is indeed the case, then, I don't think what you would like to achieve is actually safe.

Let me try to illustrate this with the following scenario:

Should client B see what has been written to the storage due to an earlier request for client A? I don't think that is desirable.

The way I see it: the concept of persistence for session storage and local storage make sense on the client but not on the server, where requests from different clients cannot be kept apart. Thus the safe option is how it currently works: a volatile storage service. This prevents data leaking from one client to another.

That doesn't mean this is necessarily a security issue for your application, however if I was to change the behavior to use a global shared storage by default, then I think it might open up potential security issues for other applications using this library.

gorkemyontem commented 5 years ago

Hi @dscheerens, I understand your concerns and I think you are right. This feature shouldn't be a part of your library.

As for my application, I use server side inMemoryStorage as a cache system. I have common cacheKeys for app content like inMemoryCache['HomePageContent'] and I have user specific keys for user content like inMemoryCache['user-{USERID}']

People who use this library could easily make mistake with this kind of usage.

Sorry to bother and thanks for the reply.