Library does not catch errors when changing localstorage

[BRMatt]

Describe the bug
If the browser's localstorage is at capacity (e.g. in chrome localstorage is capped at 5MB), then attempting to set an item in it will fail with a QuotaExceededError. Some parts of the LaunchDarkly SDK catch and disregard this error:

js-sdk-common/src/index.js

Lines 507 to 509 in 6315c36

	if (useLocalStorage && store) {
	return store.saveFlags(flags).catch(() => null); // disregard errors
	} else {

But the diagnostics section does not catch the error:

js-sdk-common/src/diagnosticEvents.js

Lines 124 to 129 in 6315c36

	function saveProperties() {
	if (platform.localStorage) {
	const props = { ...acc.getProps() };
	platform.localStorage.set(localStorageKey, JSON.stringify(props), () => {});
	}
	}

Which causes the browser to report an unhandled promise rejection. We noticed this because our frontend error tracking tool is filling up with unhandled promise rejection errors from the LaunchDarkly SDK.

Expected behavior
The diagnostics code should call .catch() on the promise, and disregard (or handle) any errors encountered while storing this information.

[eli-darkly]

Thanks for catching this. Should be a straightforward fix.

[koprivajakub]

@eli-darkly do you have any update on this. In terms of when can we expect this to be fixed?

[eli-darkly]

@koprivajakub Sorry for the delay. We included a fix for this in the 4.x version of js-sdk-common that's used by the client-side Node SDK, but we neglected to patch it in the 3.x version that's used by the JS SDK, thinking it had already been taken care of. I'll reschedule this as needing to be done.

[koprivajakub]

@eli-darkly thank you. Please keep us posted in this issue. Currently we are ignoring the error in our error reporting tool, but we would like to remove the error from ignore list as soon as it will be fixed in order to catch any other error related to the "full" localStorage.

[eli-darkly]

@koprivajakub The fix we're planning to release will still log an error message in the browser console, but it will just do it once (per SDK client instance), and there won't be any unhandled promise rejections. Does that sound reasonable?

[eli-darkly]

@koprivajakub Actually, that fix has now been released in 2.20.1. Please let me know if that works for you. If the single console error message is still disruptive to your monitoring, we could consider making an additional change to let applications turn off this kind of error logging; I think we still do want it by default, though, since it does indicate a problem that can interfere with SDK functionality.

[koprivajakub]

@eli-darkly I think the console message won't be a problem for us. On the other hand we are subscribing to errors via event hook.

LDClient.on('error', (...args) => {...})

To me it would make sense what you are proposing. Default logic would actually throw it into the console and while integrating you have an option to handle the errors in the callback of the onError event hook.

We will update the library and remove the filter on error reporting to see whether it is still a problem or not 👍 thx a lot