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

feat: Extension last reload time update #707

Merged
merged 4 commits into from Dec 26, 2016

Conversation

Projects
None yet
5 participants
@aniketkudale
Copy link
Contributor

commented Dec 24, 2016

Fixes #617

aniketkudale added some commits Dec 23, 2016

Merge pull request #2 from mozilla/master
feat: web-ext automatically checks for updates  (#676)
@coveralls

This comment has been minimized.

Copy link

commented Dec 24, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling aae9c0e on aniketkudale:feat-reload-time into 31d2e46 on mozilla:master.

@aniketkudale

This comment has been minimized.

Copy link
Contributor Author

commented Dec 24, 2016

Hi @kumar303, could you please review this, please do let me know if I am missing something here. Thanks

@kumar303

This comment has been minimized.

Copy link
Member

commented Dec 25, 2016

Hi @aniketkudale , thanks for starting this up again. Can you please address all the feedback I left in https://github.com/mozilla/web-ext/pull/685/files ? I still applies to this patch but they are just small fixes, should be straight forward. Let me know if anything is unclear.

@kumar303

This comment has been minimized.

Copy link
Member

commented Dec 25, 2016

Ah, sorry, I took a closer look at my previous feedback and I see you addressed it. Let me try this out! One sec.

@kumar303
Copy link
Member

left a comment

This is working well! Just one small change request:

If there is an error during a reload, it looks like this:

Last extension reload: 20:03:54 GMT-0600 (CST)WebExtError: reload response error: unknownError: error occurred while processing 'reload: SyntaxError: JSON.parse: expected ',' or '}' after property value in object at line 8 column 3 of the JSON data
Stack: readJSON/</<@resource://gre/modules/Extension.jsm:866:19
NetUtil_asyncFetch/<.onStopRequest@resource://gre/modules/NetUtil.jsm:128:17
Line: 866, column: 19
    at /Users/kumar/dev/web-ext/dist/webpack:/src/firefox/remote.js:80:15
    at EventEmitter.extend.handleMessage (/Users/kumar/dev/web-ext/node_modules/firefox-client/lib/client.js:161:7)
    at EventEmitter.extend.readMessage (/Users/kumar/dev/web-ext/node_modules/firefox-client/lib/client.js:220:10)
    at EventEmitter.extend.onData (/Users/kumar/dev/web-ext/node_modules/firefox-client/lib/client.js:186:16)
    at emitOne (events.js:96:13)
    at Socket.emit (events.js:188:7)
    at readableAddChunk (_stream_readable.js:177:18)
    at Socket.Readable.push (_stream_readable.js:135:10)
    at TCP.onread (net.js:542:20)
Last extension reload: 20:04:14 GMT-0600 (CST)

I'd like to make that exception more readable. Here is a suggestion:

diff --git a/src/cmd/run.js b/src/cmd/run.js
index 6f9ed7c..fbeb96b 100644
--- a/src/cmd/run.js
+++ b/src/cmd/run.js
@@ -51,6 +51,9 @@ export function defaultWatcherCreator(
       log.debug(`Reloading add-on ID ${addonId}`);
       return client.reloadAddon(addonId)
         .catch((error) => {
+          // First, add some new lines to escape from the
+          // last reload time console output.
+          log.error('\n');
           log.error(error.stack);
           throw error;
         });
@coveralls

This comment has been minimized.

Copy link

commented Dec 25, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 652b983 on aniketkudale:feat-reload-time into 31d2e46 on mozilla:master.

@aniketkudale

This comment has been minimized.

Copy link
Contributor Author

commented Dec 25, 2016

Hi @kumar303, updated the changes. Thanks.

@kumar303
Copy link
Member

left a comment

Great, thanks!

@kumar303 kumar303 merged commit b265d44 into mozilla:master Dec 26, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@aniketkudale

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2016

Thanks for merging this @kumar303

@caitmuenster

This comment has been minimized.

Copy link

commented Dec 29, 2016

Thanks, Aniket! Your contribution has been added to our Recognition wiki. We're happy to have you on the team!

@aniketkudale

This comment has been minimized.

Copy link
Contributor Author

commented Dec 29, 2016

Thanks a lot @caitmuenster :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.