Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add window or global to allow SSR compatibility #51

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

moogus
Copy link

@moogus moogus commented Dec 10, 2020

Add window or global to allow SSR compatibility

This work aims to allow the use of this module in a server side rendered environment. This work does not enable the module for use on the server but removes window dependancies.

  • add window-or-global package
  • update localStorage with global fallback

image

@@ -36,7 +44,7 @@ export default class SecureLS {
config.encodingType.toLowerCase() :
constants.EncrytionTypes.BASE64;

this.ls = localStorage;
this.ls = root.localStorage || globalStorage;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please also set the storage type on window?

this.ls = root[root.storageType || 'localStorage'] || globalStorage;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

root in this context is the window, the aim was to stop the library breaking when used in ssr enabled applications.

I'm not aware of storageType as a window property

Copy link
Author

@moogus moogus Dec 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I may not provided enough context to the PR, sorry. I've had an issue with SSR in react this is my current work around when using SecureLS:

const getSecureLS = () => {
  if (typeof window !== 'undefined') {
    return new SecureLS({});
  }
  return {
    set: () => ({}),
    get: () => ({}),
    remove: () => ({}),
  };
};

@@ -9,6 +10,13 @@ import DES from 'crypto-js/tripledes';
import RABBIT from 'crypto-js/rabbit';
import RC4 from 'crypto-js/rc4';

const globalStorage = {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be:

const globalStorage = root.customStorage || {setItem: () => {},
  getItem: () => {},
  removeItem: () => {},
  clear: () => {}
};

this will allow overriding the global object and defining your own. Thoughts?

Copy link
Author

@moogus moogus Dec 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would customStorage be?

Are you assuming this would be on the global object when used in a node environment? If so maybe it could be set as a property for use in server node apps. I'm not sure this library would be used on the server.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would help in defining custom methods(get, set, etc.) when localStorage is not present(in case of incognito mode and cookies disabled scenario). It will help there.

Copy link
Author

@moogus moogus Dec 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

localStorage is available in incognito. I'm not quite sure what scenario you are suggesting can you specify a browser etc.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@softvar any final thoughts?

@NayamAmarshe
Copy link

Any updates on this?

@softvar softvar force-pushed the master branch 7 times, most recently from e32ce42 to 7be9c85 Compare June 19, 2024 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants