Issue 29370999: Issue 4783 - Use modern JavaScript syntax for the messageResponder (Closed)
DescriptionIssue 4783 - Use modern JavaScript syntax for the messageResponder
Patch Set 1 #
Total comments: 5
Patch Set 2 : Remove pointless IIFE #Patch Set 3 : "use strict"; #
Total comments: 14
Patch Set 4 : Addressed feedback #
Total comments: 2
Patch Set 5 : Use destructuring + const, fix typo #
Total comments: 6
Patch Set 6 : Addressed further feedback #MessagesTotal messages: 22
Patch Set 1 Since the main change for #4579 added the use of some ES6 syntax and will require extensive QA I thought we should update the syntax elsewhere in the file whilst at it. https://codereview.adblockplus.org/29370999/diff/29371000/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29370999/diff/29371000/messageResponder.js... messageResponder.js:135: FilterNotifier.on(name, function() (I originally used an arrow function here but that broke things since they do not bind the arguments variable!)
https://codereview.adblockplus.org/29370999/diff/29371000/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29370999/diff/29371000/messageResponder.js... messageResponder.js:18: (global => Do we even need an IIFE with ES6? Wouldn't a simple, unconditional block have the same effect, as long as we use let (instead of var) inside the block?
Patch Set 2 : Remove pointless IIFE https://codereview.adblockplus.org/29370999/diff/29371000/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29370999/diff/29371000/messageResponder.js... messageResponder.js:18: (global => On 2017/01/11 16:29:08, Sebastian Noack wrote: > Do we even need an IIFE with ES6? Wouldn't a simple, unconditional block have > the same effect, as long as we use let (instead of var) inside the block? Good point, Done.
https://codereview.adblockplus.org/29370999/diff/29371000/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29370999/diff/29371000/messageResponder.js... messageResponder.js:18: (global => On 2017/01/13 08:44:49, kzar wrote: > On 2017/01/11 16:29:08, Sebastian Noack wrote: > > Do we even need an IIFE with ES6? Wouldn't a simple, unconditional block have > > the same effect, as long as we use let (instead of var) inside the block? > > Good point, Done. I just noticed, you'd also have to use strict mode, otherwise named functions aren't block-scoped.
Also, perhaps we should modernize the JavaScript in the rest of adblockplusui in the same go, if it is just for consistence.
Patch Set 3 : "use strict"; > Also, perhaps we should modernize the JavaScript in the rest > of adblockplusui in the same go, if it is just for > consistence. I think these changes are already getting kind of huge so I opened issue #4801 for that. https://codereview.adblockplus.org/29370999/diff/29371000/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29370999/diff/29371000/messageResponder.js... messageResponder.js:18: (global => On 2017/01/13 11:54:48, Sebastian Noack wrote: > On 2017/01/13 08:44:49, kzar wrote: > > On 2017/01/11 16:29:08, Sebastian Noack wrote: > > > Do we even need an IIFE with ES6? Wouldn't a simple, unconditional block > have > > > the same effect, as long as we use let (instead of var) inside the block? > > > > Good point, Done. > > I just noticed, you'd also have to use strict mode, otherwise named functions > aren't block-scoped. Done.
LGTM
https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js... messageResponder.js:22: var ext = require("ext_background"); Detail: It'd be great if we could avoid this if-block since it's a bit odd to see a variable defined inside a block solely for use outside of it. The same effect could be achieved using: var ext = ext || require("ext_background"); https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js... messageResponder.js:24: let port = require("messaging").port; Is there a reason not to use `const` for imported variables? The idea being that we shouldn't modify imports anyway. https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js... messageResponder.js:64: for (let key of keys) Detail: Please put braces around blocks that have multiple lines to avoid potential misinterpretation or mistakes when making changes to the code. Also applies to other loops that have been modified. https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js... messageResponder.js:125: actions.forEach(action => Since we're using `let` and for-of now, there should no longer be a need for using `Array.forEach()` anymore. Also applies to other loops that have been modified.
Patch Set 4 : Addressed feedback (I didn't test these latest changes yet, will do when I get home.) https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js... messageResponder.js:22: var ext = require("ext_background"); On 2017/01/17 11:03:51, Thomas Greiner wrote: > Detail: It'd be great if we could avoid this if-block since it's a bit odd to > see a variable defined inside a block solely for use outside of it. > > The same effect could be achieved using: > > var ext = ext || require("ext_background"); Done. https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js... messageResponder.js:24: let port = require("messaging").port; On 2017/01/17 11:03:51, Thomas Greiner wrote: > Is there a reason not to use `const` for imported variables? The idea > being that we shouldn't modify imports anyway. Well I considered that but we generally use `let` elsewhere. If it's all the same I'd rather avoid falling down the `const` vs `let` rabbit hole right now, but FWIW I agree that perhaps we should generally use `const` much of the time we currently use `let`. https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js... messageResponder.js:64: for (let key of keys) On 2017/01/17 11:03:50, Thomas Greiner wrote: > Detail: Please put braces around blocks that have multiple lines to avoid > potential misinterpretation or mistakes when making changes to the code. > > Also applies to other loops that have been modified. Done. https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js... messageResponder.js:125: actions.forEach(action => On 2017/01/17 11:03:51, Thomas Greiner wrote: > Since we're using `let` and for-of now, there should no longer be a need for > using `Array.forEach()` anymore. > > Also applies to other loops that have been modified. Done.
(Sorry I uploaded the wrong diff there, deleted that patchset and uploaded it again!)
https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js... messageResponder.js:24: let port = require("messaging").port; On 2017/01/17 11:25:29, kzar wrote: > On 2017/01/17 11:03:51, Thomas Greiner wrote: > > Is there a reason not to use `const` for imported variables? The idea > > being that we shouldn't modify imports anyway. > > Well I considered that but we generally use `let` elsewhere. > > If it's all the same I'd rather avoid falling down the `const` vs `let` rabbit > hole right now, but FWIW I agree that perhaps we should generally use `const` > much of the time we currently use `let`. If it wouldn't be for the consistency with our existing code, I'd agree that using const for imports sounds reasonable. On the other hand, we are currently modernizing the code in adblockpluschrome as well. So now would be a good time to come up with a guideline for using const. But I'm feeling indecisive what the general rule for using const should be. So far we use const for global variables of immutable type, that could be inlined as well, but have been put into variables in order to improve the maintainability. While imported modules are not immutable, they are similar in the way that it makes the code more maintainable (and also somewhat faster) when we put them into variables rather than inlining/duplicating require(module).symbol everywhere, and there isn't much of a point to reassign a variable holding a module either. While I'm all for a rule that suggests using const in those scenarios, I might be opposed to a rule that suggests using const wherever possible. For example, consider following code: let items = []; for (let x of y) items.push(y.getValue()); Since "items" isn't reassigned, we could use const instead of let here. But IMO this would already be weird enough, since the value of "items" is changing though. Now imagine that the logic is changed later so that under some circumstances the array has to be cleared: let items = []; for (let x of y) if (x.isFirst) items = []; items.push(y.getValue()); Now we suddenly have a reassignment, and need to adapt the variable declaration to use let again. The other way around, if code like this gets changed so that we no longer reassign we would have to adapt the declaration to use const. A rule that would require that seems quite impracticable to me. This is somewhat similar to the question when to use tuples, and when to use lists, in Python. Initially, we preferred to use tuples whenever possible (i.e for every sequence that isn't modified by the current code). However, this caused the same mess I'm afraid of here. So we now have a rule for our Python code, to use tuples for data that have structure, and lists for data that have order. Or if you put it more general, use immutability for that kind of data which have no point in changing them regardless of the current context.
https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js... messageResponder.js:24: let port = require("messaging").port; On 2017/01/17 15:37:00, Sebastian Noack wrote: > On 2017/01/17 11:25:29, kzar wrote: > > On 2017/01/17 11:03:51, Thomas Greiner wrote: > > > Is there a reason not to use `const` for imported variables? The idea > > > being that we shouldn't modify imports anyway. > > > > Well I considered that but we generally use `let` elsewhere. > > > > If it's all the same I'd rather avoid falling down the `const` vs `let` rabbit > > hole right now, but FWIW I agree that perhaps we should generally use `const` > > much of the time we currently use `let`. > > If it wouldn't be for the consistency with our existing code, I'd agree that > using const for imports sounds reasonable. On the other hand, we are currently > modernizing the code in adblockpluschrome as well. So now would be a good time > to come up with a guideline for using const. > > But I'm feeling indecisive what the general rule for using const should be. So > far we use const for global variables of immutable type, that could be inlined > as well, but have been put into variables in order to improve the > maintainability. While imported modules are not immutable, they are similar in > the way that it makes the code more maintainable (and also somewhat faster) when > we put them into variables rather than inlining/duplicating > require(module).symbol everywhere, and there isn't much of a point to reassign a > variable holding a module either. > > While I'm all for a rule that suggests using const in those scenarios, I might > be opposed to a rule that suggests using const wherever possible. I agree that we should be careful and only use `const` for some few well-defined cases. It shouldn't be too difficult to define such unambiguous scenarios (e.g. module imports or `const MILLIS_IN_SECOND = 1000;`) for which it makes sense.
https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js... messageResponder.js:24: let port = require("messaging").port; On 2017/01/17 15:48:40, Thomas Greiner wrote: > On 2017/01/17 15:37:00, Sebastian Noack wrote: > > On 2017/01/17 11:25:29, kzar wrote: > > > On 2017/01/17 11:03:51, Thomas Greiner wrote: > > > > Is there a reason not to use `const` for imported variables? The idea > > > > being that we shouldn't modify imports anyway. > > > > > > Well I considered that but we generally use `let` elsewhere. > > > > > > If it's all the same I'd rather avoid falling down the `const` vs `let` > rabbit > > > hole right now, but FWIW I agree that perhaps we should generally use > `const` > > > much of the time we currently use `let`. > > > > If it wouldn't be for the consistency with our existing code, I'd agree that > > using const for imports sounds reasonable. On the other hand, we are currently > > modernizing the code in adblockpluschrome as well. So now would be a good time > > to come up with a guideline for using const. > > > > But I'm feeling indecisive what the general rule for using const should be. So > > far we use const for global variables of immutable type, that could be inlined > > as well, but have been put into variables in order to improve the > > maintainability. While imported modules are not immutable, they are similar in > > the way that it makes the code more maintainable (and also somewhat faster) > when > > we put them into variables rather than inlining/duplicating > > require(module).symbol everywhere, and there isn't much of a point to reassign > a > > variable holding a module either. > > > > While I'm all for a rule that suggests using const in those scenarios, I might > > be opposed to a rule that suggests using const wherever possible. > > I agree that we should be careful and only use `const` for some few well-defined > cases. It shouldn't be too difficult to define such unambiguous scenarios (e.g. > module imports or `const MILLIS_IN_SECOND = 1000;`) for which it makes sense. Rather than listing specific use cases in our coding styles, I'd like to have a generic rule, but one that accounts for my concerns above. Perhaps something along the lines of: Use `const` for constant expressions that could as well have been inlined. If it is helpful we could add the know use cases as examples: Use `const` for constant expressions that could as well have been inlined (e.g. static parameters, imported modules, etc.).
https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js... messageResponder.js:24: let port = require("messaging").port; On 2017/01/17 17:15:48, Sebastian Noack wrote: > Rather than listing specific use cases in our coding styles, I'd like to have a > generic rule, but one that accounts for my concerns above. Perhaps something > along the lines of: > > Use `const` for constant expressions that could as well have been inlined. > > If it is helpful we could add the know use cases as examples: > > Use `const` for constant expressions that could as well have been inlined > (e.g. static parameters, imported modules, etc.). Sounds good.
On 2017/01/17 17:20:09, Thomas Greiner wrote: > https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js > File messageResponder.js (right): > > https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js... > messageResponder.js:24: let port = require("messaging").port; > On 2017/01/17 17:15:48, Sebastian Noack wrote: > > Rather than listing specific use cases in our coding styles, I'd like to have > a > > generic rule, but one that accounts for my concerns above. Perhaps something > > along the lines of: > > > > Use `const` for constant expressions that could as well have been inlined. > > > > If it is helpful we could add the know use cases as examples: > > > > Use `const` for constant expressions that could as well have been inlined > > (e.g. static parameters, imported modules, etc.). > > Sounds good. Dave, if you agree, as you are already working on some additions to our coding practices mind adding this?
https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js... messageResponder.js:24: let port = require("messaging").port; On 2017/01/17 17:20:08, Thomas Greiner wrote: > On 2017/01/17 17:15:48, Sebastian Noack wrote: > > Rather than listing specific use cases in our coding styles, I'd like to have > a > > generic rule, but one that accounts for my concerns above. Perhaps something > > along the lines of: > > > > Use `const` for constant expressions that could as well have been inlined. > > > > If it is helpful we could add the know use cases as examples: > > > > Use `const` for constant expressions that could as well have been inlined > > (e.g. static parameters, imported modules, etc.). > > Sounds good. (Sorry, I first replied in the globally. Copying this comment where it belongs.) Dave, if you agree, as you are already working on some additions to our coding practices mind adding this? https://codereview.adblockplus.org/29370999/diff/29372229/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29370999/diff/29372229/messageResponder.js... messageResponder.js:23: let port = require("messaging").port; Also what is about using destructing here?
Patch Set 5 : Use destructuring + const, fix typo https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js... messageResponder.js:24: let port = require("messaging").port; On 2017/01/17 17:30:16, Sebastian Noack wrote: > Dave, if you agree, as you are already working on some additions to > our coding practices mind adding this? Sure that all sound sensible enough to me. I've updated this review to use const and will update the coding practices review now. https://codereview.adblockplus.org/29370999/diff/29372229/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29370999/diff/29372229/messageResponder.js... messageResponder.js:23: let port = require("messaging").port; On 2017/01/17 17:30:16, Sebastian Noack wrote: > Also what is about using destructing here? Done.
LGTM
https://codereview.adblockplus.org/29370999/diff/29372263/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29370999/diff/29372263/messageResponder.js... messageResponder.js:30: const {NotificationStorage} = require("notification"); This doesn't work. What you'd want to do is: const {Notification: NotificationStorage} = require("notification"); https://codereview.adblockplus.org/29370999/diff/29372263/messageResponder.js... messageResponder.js:141: }; Detail: This semicolon is still left over from your previous change. https://codereview.adblockplus.org/29370999/diff/29372263/messageResponder.js... messageResponder.js:239: const hostname = sender.frame.url.hostname; Detail: I don't see a compelling enough use-case for using `const` here. The `hostname` variable should be allowed to change to allow for maximum flexibility when maintaining this part of the code.
Patch Set 6 : Addressed further feedback https://codereview.adblockplus.org/29370999/diff/29372263/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29370999/diff/29372263/messageResponder.js... messageResponder.js:30: const {NotificationStorage} = require("notification"); On 2017/01/18 11:40:09, Thomas Greiner wrote: > This doesn't work. What you'd want to do is: > > const {Notification: NotificationStorage} = require("notification"); Done. https://codereview.adblockplus.org/29370999/diff/29372263/messageResponder.js... messageResponder.js:141: }; On 2017/01/18 11:40:09, Thomas Greiner wrote: > Detail: This semicolon is still left over from your previous change. Whoops, Done. https://codereview.adblockplus.org/29370999/diff/29372263/messageResponder.js... messageResponder.js:239: const hostname = sender.frame.url.hostname; On 2017/01/18 11:40:09, Thomas Greiner wrote: > Detail: I don't see a compelling enough use-case for using `const` here. The > `hostname` variable should be allowed to change to allow for maximum flexibility > when maintaining this part of the code. Done.
LGTM
LGTM 
|
