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.

Sign up for GitHub

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

fix #339 by normalizing localStorage and browser.storage.local usage #395

Closed
wants to merge 1 commit into from
Closed

fix #339 by normalizing localStorage and browser.storage.local usage #395

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 56 additions & 37 deletions src/browser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,47 @@ exports.formatArgs = formatArgs;
exports.save = save;
exports.load = load;
exports.useColors = useColors;
exports.storage = 'undefined' != typeof chrome
&& 'undefined' != typeof chrome.storage
? chrome.storage.local
: localstorage();

exports.storage = (function() {
var browser = window.browser || window.chrome;

if (typeof browser !== 'undefined' &&
typeof browser.storage !== 'undefined' &&
typeof browser.storage.local !== 'undefined') {

// maintain compatibility with localStorage
return {
get: function(key, cb) {
browser.storage.local.get(key, function(items) {
cb(items[key]);
});
},

set: function(key, value) {
var items = {};
items[key] = value;
browser.storage.local.set(items);
},

remove: browser.storage.local.remove
};
}

if (window.localStorage) {
return {
get: function(key, cb) {
var value = window.localStorage.getItem(key);
cb(value);
},
set: function(key, value) {
window.localStorage.setItem(key, value);
},
remove: function(key) {
window.localStorage.removeItem(key);
}
};
}
})();

/**
* Colors.
Expand Down Expand Up @@ -131,13 +168,11 @@ function log() {
*/

function save(namespaces) {
try {
if (null == namespaces) {
exports.storage.removeItem('debug');
} else {
exports.storage.debug = namespaces;
}
} catch(e) {}
if (null == namespaces) {
exports.storage.remove('debug');
} else {
exports.storage.set('debug', namespaces);
Copy link

Choose a reason for hiding this comment

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

Firefox: Error: Incorrect argument types for storage.StorageArea.set.

}
}

/**
Expand All @@ -148,35 +183,19 @@ function save(namespaces) {
*/

function load() {
try {
return exports.storage.debug;
} catch(e) {}

// If debug isn't set in LS, and we're in Electron, try to load $DEBUG
if (typeof process !== 'undefined' && 'env' in process) {
return process.env.DEBUG;
if (exports.storage) {
exports.storage.get('debug', exports.enable);
Copy link

Choose a reason for hiding this comment

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

Another problem I am seeing is that enabled is running before enable runs here, and thus exports.names are not set when it is checking for what to enable. This breaks the enabled check and ensures logging is disabled for the duration of the page load.

Copy link
Author

Choose a reason for hiding this comment

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

I don't follow you here at all. There's no enabled and there's no exports.names. Care to rephrase?

Copy link

Choose a reason for hiding this comment

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

Sorry, I'll elaborate:

There's no enabled and there's no exports.names

Not in this file, no. We need to jump over to debug.js to understand what happens. In that file there is a method called enabled. It is run from createDebug, i.e. when debug('foo') is called. Currently, we're in a situation where enabled is very likely to be called before exports.names is set, which is a precondition of that method. Thus we are breaking the contract of enabled.

exports.names are set by enable, which is passed as a callback in load. Problem is, now that storage.get is async, the order which the assignment happens is mixed up. With localStorage we can assume synchronous behavior and it works, but this is not the case for WebExtensions.

import debug from 'debug';
const log = debug('foo');

log('Hello!'); // <-- Never logs because debug was not enabled when `debug('foo')` was called

setTimeout(() => {
  debug('bar')('Hi!'); // <-- (Probably) runs because the promise has resolved and the callback fired
}, 1000);

Copy link
Author

Choose a reason for hiding this comment

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

In other words, you're pointing out point no. 2 in the description of this PR

in browsers apps/extensions, due to the async nature, calling debug before we read from browser.storage.local may end up in lost messages

Comments:

  1. our workaround is to queue up messages, and process them after the local storage value is read
  2. I see this fixed as part of another issue/PR (the goal is to support chrome.storage.local properly now)
  3. the risk is rather small that messages are lost, given that the main execution is deferred (setTimeout 0)

Copy link

Choose a reason for hiding this comment

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

That is correct, I am indeed pointing that out.

Basically what you are arguing is that this PR is making the situation better, as debug goes from not working at all to working in most cases. I can buy that, but I don't buy that the risk is small. I 100% consistently saw that messages that were logged by a logger assigned directly after the import debug from 'debug'; did not appear.

Let's make it clear that I'm not saying it's a deal breaker. It would be awesome to debug working. We just may need to make clear that the following is the way you need to be logging in a WebExtension until we can improve it further. Example using React.js:

import debug from 'debug';
// const log = debug('FooComponent'); // <-- DO NOT DO THIS

export default function FooComponent() {
  debug('FooComponent')('Rendering FooComponent');
  // log('Rendering FooComponent'); // <-- DO NOT DO THIS

  return <h1>Foo</h1>;
}

} else {
if (typeof process !== 'undefined' &&
typeof process.env !== 'undefined' &&
typeof process.env.DEBUG !== 'undefined') {
exports.enable(process.env.DEBUG);
}
}
}

/**
* Enable namespaces listed in `localStorage.debug` initially.
* Enable namespaces listed in the storage `debug`` initially.
*/

exports.enable(load());

/**
* Localstorage attempts to return the localstorage.
*
* This is necessary because safari throws
* when a user disables cookies/localstorage
* and you attempt to access it.
*
* @return {LocalStorage}
* @api private
*/

function localstorage() {
try {
return window.localStorage;
} catch (e) {}
}
exports.load();