chore: setup localstorage dynamic polyfill for node env #1963
Conversation
| if (typeof window === "undefined") { | ||
| try { | ||
| const { LocalStorage } = await import("node-localstorage"); | ||
| global.localStorage = new LocalStorage("./mock-localstorage"); |
There was a problem hiding this comment.
Is there any benefit of providing this to NodeJS?
To me just ignoring local storage is already good enough, better way would be to prevent it from running in NodeJS.
There was a problem hiding this comment.
To me just ignoring local storage is already good enough
ignoring would mean that we there would still be a failing action, and an error log associated with it which imo isn't ideal
better way would be to prevent it from running in NodeJS
which would imply we disallow usage of local-peer-cache-discovery in nodejs environments which is relatively more trivial, and also disables the functionality
very straightforward to use a mock localstorage (mock DB of sorts) instead with dynamic polyfilling approach in the PR
There was a problem hiding this comment.
I am concerned about us trying to add polyfills to library for the browser only - this can lead to unintentional leak of the polyfill to browser. Did you check this?
failing action, and an error log associated
as for this - we can do a better logging then and add getter for localStorage with embedded error handling
how does this sound?
There was a problem hiding this comment.
I am concerned about us trying to add polyfills to library for the browser only
We are introducing the polyfill for nodejs, not browsers. node-localstorage is a nodejs dependency that creates a mock localstorage.
this can lead to unintentional leak of the polyfill to browser. Did you check this?
Yes - the browser tests do check for this potential leak.
danisharora099
force-pushed
the
fix/local-storage-node
branch
from
e30bc95
to
a54cf17
Compare
|
closing this PR in favor of deprioritising nodejs |
Problem
Reference to #1953, we see local storage related functionality breaking on node environments because
localStorageis undefined.While js-waku is targeted for browser environments, we need to offer flexibility in terms of its usage in node environment without things breaking.
Solution
Allow dynamic imports to polyfill
localStoragein node environments.Notes
Contribution checklist:
!in title if breaks public API;