elgerlambert / redux-localstorage

Store enhancer that syncs (a subset) of your Redux store state to localstorage.
MIT License
1.32k stars 107 forks source link

Handle localStorage if not defined (isomorphic) #13

Closed ruprict closed 8 years ago

ruprict commented 9 years ago

We get the following with the 1.0 breaking changes branch when firing up the server:

/Users/whatevs/projects/stuff/ui/node_modules/redux-localstorage/lib/persistState.js:29
var defaultStorage = (0, _adaptersLocalStorage2['default'])(localStorage);
                                                            ^
ReferenceError: localStorage is not defined
    at Object.<anonymous> (/Users/whatevs/projects/stuff/ui/node_modules/redux-localstorage/lib/persistState.js:29:61)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Module.require (module.js:364:17)
    at require (module.js:380:17)
    at Object.<anonymous> (/Users/whatevs/projects/stuff/ui/node_modules/redux-localstorage/lib/index.js:17:23)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
 +407ms

I presume this is b/c of the server-side rendering. So, I added this to fix it in persistState.js:

if (typeof window === "undefined" ) {
  require('fs');
  var LocalStorage = require('node-localstorage').LocalStorage;
  localStorage = new LocalStorage('./scratch');
} else {
  localStorage = window.localStorage;
}
// this is existing 
var defaultStorage = (0, _adaptersLocalStorage2['default'])(localStorage);

It adds a dependency on node-localstorage, but that doesn't seem too unreasonable...does it?

Thoughts?

barakcoh commented 9 years ago

I'm having the same issue but w/ React Native so the proposed solution doesn't really work for me.

ruprict commented 9 years ago

@barakcoh could we create a stub that just noops get and put? Or is there an equivalent for native that you know of?

barakcoh commented 9 years ago

@ruprict, React Native has Async Storage however, it's async so not sure if it's a good fit.

ruprict commented 9 years ago

Well, there's an Async adapter for this module. The real issue is that it defaults to LocalStorage when bootstrapping. Maybe we should do a test for what's available or not default it at all?

rt2zz commented 9 years ago

what is the desired behaivor here? I would imagine you would not compose persistState at all server side. I suppose on the server side it could be used as some sort of cache...

elgerlambert commented 9 years ago

Hi guys, thanks for raising this issue!

The straightforward fix, I think, is to remove 'const defaultStorage = adapter(localStorage);' at the top of persistState.js and replace the remaining three 'defaultStorage' values with 'adapter(localStorage)'. This ensures all 'adapter(localStorage)' assignments only happen if no storage is specified. This is effectively the same as it was before (before this became an issue). Would this solve/provide a clear solution for all (edge) cases?

My concern with a noop type solution is that this will lead to silent failures, thus leading to unexpected behaviour.

barakcoh commented 9 years ago

Sounds good to me. a footnote in the docs should help avoid the silent failures.

ruprict commented 9 years ago

:+1:

igl commented 8 years ago

Even with this fixed i get:

Warning: React attempted to reuse markup in a container but the checksum was invalid. This generally means that you are using server rendering and the markup generated on the server was not what the client was expecting.

Using the local storage in a store enhancer is probably not a good way to do this unisomorphical.

elgerlambert commented 8 years ago

Hi @igl, are you able to provide a little more info about your setup?

Redux-localstorage shouldn't effect the initial markup since redux's init action is dispatched first. An action to rehydrate the locally persisted data is dispatched thereafter.. I wonder though whether, due to the synchronous nature of localStorage the changes might be batched together.. (You could test this by writing a storage enhancer that makes the get call async by adding a timeout.)

Using the callback argument of persistState to delay rendering of your app until it is finished rehydrating the store will also cause a difference between what was rendered server side and what is initially rendered client side. Are you utilising the callback argument?

igl commented 8 years ago

Hey, No i wasn't using a callback. I only defined a .slicer and a .merge. I no longer implicitly store stuff in local storage now but instead save and load from within a action creator. Just like you would do it talking with your backend api. I feel it's a nicer way to contain all side effects in the action creator.

elgerlambert commented 8 years ago

Hi @igl, thanks for replying. Based on your comment it's clear you were using 0.4.0 whereas my previous comment and the fix proposed in this issue refers to 1.0.0-rc4, which has a significantly different API.

You're right about 0.4.0, it doesn't support "unimorphical" apps since store rehydration happens synchronously (before your app has a chance to render the initial markup), 1.0.0 will allow you to perform server-side rendering (previous rc/beta versions already did), without getting in the way.

Kuzmin commented 8 years ago

I'm on 1.0.0-rc4 on React Native and I still get

Can't find variable: localStorage
persistState.js:29

Is this still expected and am I supposed to patch this myself? Just curious since I didn't feel it was clear from the above conversation if this should be fixed or not yet.

mdziekon commented 8 years ago

@Kuzmin - I've just made a PR, based on one of the comments above, which should solve this issue. I've tested it on my node.js app build, but couldn't test it on React Native (as I have never used it before).

@elgerlambert - Could you please check my PR and include it in the next RC release if it's ok with you? It also might be worth noting in the documentation, that for non-browser environments defaultStorage has to be overridden (or otherwise it will fail to build)...

Kuzmin commented 8 years ago

@mdziekon I looked at the code that Babel outputs from your source and it looks like that would solve the problem!

hoodsy commented 8 years ago

@elgerlambert is there any solution for this? I'm getting the same issue (on rc4)

mdziekon commented 8 years ago

@hoodsy My PR (fixing the problem) got merged already, but apparently it has not been deployed to npm yet.

hoodsy commented 8 years ago

@mdziekon ah ok. thanks, was hard to tell from the compiled code.

hoodsy commented 8 years ago

@elgerlambert any plans to push the update to npm? I still get the error:

node_modules/redux-localstorage/lib/persistState.js:29
var defaultStorage = (0, _adaptersLocalStorage2['default'])(localStorage);
                                                            ^

ReferenceError: localStorage is not defined
elgerlambert commented 8 years ago

1.0.0-rc5 is out and includes this fix! My apologies that it took so long.

azz0r commented 7 years ago

FYI using that RC I get

/Users/azz0r/Sites/Personal/react-base-kit/node_modules/redux-localstorage/lib/persistState.js:73
      finalStorage.get(finalKey, function (err, persistedState) {
                  ^

TypeError: Cannot read property 'get' of undefined
    at /Users/azz0r/Sites/Personal/react-base-kit/node_modules/redux-localstorage/lib/persistState.js:73:19
Kuzmin commented 7 years ago

@azz0r When using that RC in an environment where you don't have access to localStorage, you will need to explicitly pass in an adapter for the storage module that you want to use. Does this seem like it could be the issue?

Rajat421 commented 7 years ago

hi i want to make request to get data in getintialprops() method of next.js which execute at server side but to fetch data i have to send auth token in headers which was stored in local storage ....,so how can i get auth token at server side ..

nataliyakarat commented 6 years ago

it doesn't work for me "redux-localstorage": "^1.0.0-rc5",

ReferenceError: window is not defined
    at getDefaultStorage(..../persistState.js:30:42)