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

andreineculau · 2016-12-24T19:00:50Z

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

coveralls · 2016-12-24T19:03:08Z

Coverage remained the same at 63.462% when pulling fe77c4b on andreineculau:patch-1 into 280a64d on visionmedia:master.

thebigredgeek · 2016-12-26T02:34:02Z

I'd feel better merging this if we had some test coverage around local storage. This seems like an accident waiting to happen without it :/. Is there any way you could add tests around the original code and make sure all is well after plugging in your changes? We need better coverage anyway :p

eliihen · 2017-01-14T20:08:18Z

I just applied this patch, and can report the following:

Before: debug in my WebExtension is logging absolutely nothing, no matter where I set debug to *.
After: debug logs as normal, but it is reading from localStorage, which doesn't look like what was intended.

Any chance of this getting updated and merged?

eliihen · 2017-01-14T20:08:31Z

src/browser.js

+    if (browser.storage.local.get.length === 2) {
+      // Chrome/Opera/Edge use the callback pattern
+      return browser.storage.local;
+    } else if (browser.storage.local.get.length === 1) {


When testing in a Firefox 52a2 WebExtension, this returns 0, which means Firefox falls back to localStorage.

@esphen true that, I've seen the same (arity is 0 because they implement the functions with dynamic arguments).
The PR has been amended with a commit my team had in our private fork.

andreineculau · 2017-01-14T20:35:11Z

@thebigredgeek I agree with you on increasing the coverage, but it won't be me fixing that. Sorry.

eliihen · 2017-01-14T20:38:23Z

src/browser.js

+  if (null == namespaces) {
+    exports.storage.remove('debug');
+  } else {
+    exports.storage.set('debug', namespaces);


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

coveralls · 2017-01-14T20:38:54Z

Coverage remained the same at 63.75% when pulling 6e0d190 on andreineculau:patch-1 into 1c163a4 on visionmedia:master.

andreineculau · 2017-01-14T21:19:37Z

@esphen better? thanks for debugging! (I cannot test at the moment)

coveralls · 2017-01-14T21:20:08Z

Coverage remained the same at 63.75% when pulling 0dac9b6 on andreineculau:patch-1 into 1c163a4 on visionmedia:master.

eliihen · 2017-01-14T21:37:04Z

No problem söta bror, I'm just as interested in seeing this land as you!

Yep, this is progress. No longer seeing Incorrect argument types for storage.StorageArea.set there.

However now I am seeing TypeError: (intermediate value).split is not a function at this line. namespaces is coming into the enable method as the following, which does not support being split as a string:

{ debug: "*" }

eliihen · 2017-01-14T21:45:49Z

I think storage.get isn't quite right according to the return value of the promise. Observe the following:

> browser.storage.local.set({ 'debug': '*' })
< Promise { <state>: "pending" }
> browser.storage.local.get('debug').then(console.log)
< Promise { <state>: "pending" }
< Object { debug: "*" }

eliihen

We got some more issues to resolve before this will work. Most notable the wonky user-agent check and some async issues.

eliihen · 2017-01-14T22:31:12Z

src/browser.js

+
+if (typeof navigator !== 'undefined' &&
+    typeof navigator.userAgent !== 'undefined') {
+  isFirefox = navigator.userAgent.toLowerCase().match(/firefox\/(\d+)/)


This check is a bit wonky for me because I am using the random agent spoofer extension. Suddenly it stopped working for no particular reason, as it randomizes the agent every now and then.

Can we find a better way to check than this?

Try my patch-1-without-ua-sniff branch https://github.com/andreineculau/debug/tree/patch-1-without-ua-sniff although I find it utterly wrong for an extension to do that even for other extensions.

Good news: I just tested the following in Firefox:

> browser.storage.local.get('debug', console.log) < undefined < Object { debug: "*" }

In other words, you don't actually need any browser detection here, as Firefox supports the callback pattern in addition to the promise pattern.

eliihen · 2017-01-14T22:36:09Z

src/browser.js

+        // Firefox uses the Promise pattern
+        get: function(key, cb) {
+          browser.storage.local.get(key).then(cb);
+        },


This needs to be

get: function(key, cb) { browser.storage.local.get(key).then(function(result) { cb(result && result.debug); }); },

Or similar

eliihen · 2017-01-14T22:46:36Z

src/browser.js

-  if (typeof process !== 'undefined' && 'env' in process) {
-    return process.env.DEBUG;
+  if (exports.storage) {
+    exports.storage.get('debug', exports.enable);


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.

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

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);

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:

our workaround is to queue up messages, and process them after the local storage value is read

I see this fixed as part of another issue/PR (the goal is to support chrome.storage.local properly now)

the risk is rather small that messages are lost, given that the main execution is deferred (setTimeout 0)

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>; }

coveralls · 2017-01-15T00:12:23Z

Coverage remained the same at 63.75% when pulling 1051435 on andreineculau:patch-1 into 1c163a4 on visionmedia:master.

andreineculau · 2017-01-15T00:14:03Z

@esphen storage.get should be fixed now

…l usage

coveralls · 2017-01-15T17:30:06Z

Coverage remained the same at 63.75% when pulling ead58f8 on andreineculau:patch-1 into 1c163a4 on visionmedia:master.

andreineculau · 2017-01-15T17:31:00Z

> In other words, you don't actually need any browser detection here, as Firefox supports the callback pattern *in addition* to the promise pattern.

@esphen I take your word on Firefox's behaviour (although I haven't read anything as such about it). The current PR reflects that.

…

-- andreineculau.com <http://www.andreineculau.com>

eliihen

Verified that the current patch works as intended in my WebExtension.

Provided the maintainers (@thebigredgeek?) is okay with the known bug mentioned in the review, I don't see why we shouldn't merge this and get support for WebExtensions.

thebigredgeek · 2017-04-05T22:23:08Z

@andreineculau are we sure this is still necessary? A lot of fixes have been applied since January :p

andreineculau · 2017-04-06T05:19:36Z

@thebigredgeek oh, in that case, no

andreineculau · 2017-04-06T15:43:42Z

FWIW and posterity that was pure 100% irony

…

On Thu, Apr 6, 2017 at 5:35 PM, Andrew E. Rhyne ***@***.***> wrote: Closed #395 <#395>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#395 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AArOQVOjWqCX3bVmqEwhET28sWAaea5gks5rtQY4gaJpZM4LVR0V> .

-- andreineculau.com <http://www.andreineculau.com>

scholtzm · 2017-04-25T09:01:03Z

Seems like the initial PR (#143) that introduced chrome.storage.local was never really tested since the API is async (docs) and completely different to localStorage. It also managed to break WebExtensions.

This lead to another PR (#190) which just added extra complexity to API and is not even documented anywhere.

If we reverted pull request #143, WebExtensions support would be restored and we would also remove "support" for Chrome Apps which never really worked.

What do you think?

eliihen reviewed Jan 14, 2017

View reviewed changes

andreineculau force-pushed the patch-1 branch from fe77c4b to a54770f Compare January 14, 2017 20:30

andreineculau force-pushed the patch-1 branch from a54770f to 6e0d190 Compare January 14, 2017 20:36

eliihen reviewed Jan 14, 2017

View reviewed changes

andreineculau force-pushed the patch-1 branch from 6e0d190 to 0dac9b6 Compare January 14, 2017 21:18

eliihen suggested changes Jan 14, 2017

View reviewed changes

andreineculau force-pushed the patch-1 branch from 0dac9b6 to 1051435 Compare January 15, 2017 00:09

fix debug-js#339 by normalizing localStorage and browser.storage.loca…

ead58f8

…l usage

andreineculau force-pushed the patch-1 branch from 1051435 to ead58f8 Compare January 15, 2017 17:28

eliihen approved these changes Jan 15, 2017

View reviewed changes

thebigredgeek closed this Apr 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

andreineculau commented Dec 24, 2016

coveralls commented Dec 24, 2016 •

edited

Loading

thebigredgeek commented Dec 26, 2016

eliihen commented Jan 14, 2017

eliihen Jan 14, 2017

andreineculau Jan 14, 2017

andreineculau commented Jan 14, 2017

eliihen Jan 14, 2017

coveralls commented Jan 14, 2017 •

edited

Loading

andreineculau commented Jan 14, 2017

coveralls commented Jan 14, 2017 •

edited

Loading

eliihen commented Jan 14, 2017

eliihen commented Jan 14, 2017

eliihen left a comment

eliihen Jan 14, 2017

andreineculau Jan 15, 2017

eliihen Jan 15, 2017

eliihen Jan 14, 2017

eliihen Jan 14, 2017

andreineculau Jan 15, 2017

eliihen Jan 15, 2017

andreineculau Jan 15, 2017

eliihen Jan 15, 2017

coveralls commented Jan 15, 2017 •

edited

Loading

andreineculau commented Jan 15, 2017

coveralls commented Jan 15, 2017 •

edited

Loading

andreineculau commented Jan 15, 2017 via email

eliihen left a comment

thebigredgeek commented Apr 5, 2017

andreineculau commented Apr 6, 2017 •

edited

Loading

andreineculau commented Apr 6, 2017 via email

scholtzm commented Apr 25, 2017 •

edited

Loading

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

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

Conversation

andreineculau commented Dec 24, 2016

coveralls commented Dec 24, 2016 • edited Loading

thebigredgeek commented Dec 26, 2016

eliihen commented Jan 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreineculau commented Jan 14, 2017

Choose a reason for hiding this comment

coveralls commented Jan 14, 2017 • edited Loading

andreineculau commented Jan 14, 2017

coveralls commented Jan 14, 2017 • edited Loading

eliihen commented Jan 14, 2017

eliihen commented Jan 14, 2017

eliihen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 15, 2017 • edited Loading

andreineculau commented Jan 15, 2017

coveralls commented Jan 15, 2017 • edited Loading

andreineculau commented Jan 15, 2017 via email

eliihen left a comment

Choose a reason for hiding this comment

thebigredgeek commented Apr 5, 2017

andreineculau commented Apr 6, 2017 • edited Loading

andreineculau commented Apr 6, 2017 via email

scholtzm commented Apr 25, 2017 • edited Loading

coveralls commented Dec 24, 2016 •

edited

Loading

coveralls commented Jan 14, 2017 •

edited

Loading

coveralls commented Jan 14, 2017 •

edited

Loading

coveralls commented Jan 15, 2017 •

edited

Loading

coveralls commented Jan 15, 2017 •

edited

Loading

andreineculau commented Apr 6, 2017 •

edited

Loading

scholtzm commented Apr 25, 2017 •

edited

Loading