Issue 29399569: Issue 5060 - Move require into modules template, make info a module (Closed)
|
Created:
March 31, 2017, 5:44 a.m. by kzar Modified:
April 2, 2017, 5:24 a.m. Reviewers:
Sebastian Noack Visibility:
Public. |
DescriptionIssue 5060 - Move require into modules template, make info a module
Patch Set 1 #Patch Set 2 : Don't redeclare require #
Total comments: 9
Patch Set 3 : Addressed some feedback #
Total comments: 7
Patch Set 4 : Addressed more feedback #
Total comments: 7
Patch Set 5 : Improve check for require #
Total comments: 2
MessagesTotal messages: 13
Patch Set 1
https://codereview.adblockplus.org/29399569/diff/29399595/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29399569/diff/29399595/packagerChrome.py#n... packagerChrome.py:145: info_module = None How about merging chromeInfo.js.tmpl, geckoInfo.js.tmpl and modules.js.tmpl into one template: require.modules["info"] = function(module, exports) { {%- if type == "gecko-webext" %} ... {%- elif type == "edge" %} ... {%- else %} ... {%- endif %} exports.addonName = {{ metadata.get('general', 'basename')|json }}; exports.addonVersion = {{ version|json }}; exports.application = application; exports.applicationVersion = applicationVersion; exports.platform = platform; exports.platformVersion = platformVersion; return exports; }; {%- for module_name, script in modules -%} require.modules[{{ module_name|json }}] = function(module, exports) { {{ script }} return module.exports; }; {% endfor -%} That way, we... 1. simplify the logic here in the packager. 2. remove some duplication from the info module's code. 3. unify how different targets are handled (currently the logic for Edge is merged into the one for Chrome, while FF/WebExt have an independent implementation). https://codereview.adblockplus.org/29399569/diff/29399595/templates/chromeInf... File templates/chromeInfo.js.tmpl (right): https://codereview.adblockplus.org/29399569/diff/29399595/templates/chromeInf... templates/chromeInfo.js.tmpl:55: module.exports = { Replacing module.exports isn't necessary: exports.addonName = {{ metadata.get('general', 'basename')|json }}; exports.addonVersion = {{ version|json }}; exports.application = application; exports.applicationVersion = applicationVersion; exports.platform = platform; exports.platformVersion = platformVersion; https://codereview.adblockplus.org/29399569/diff/29399595/templates/modules.j... File templates/modules.js.tmpl (right): https://codereview.adblockplus.org/29399569/diff/29399595/templates/modules.j... templates/modules.js.tmpl:2: if (!("require" in window)) The assumption of the global object being "window" could be avoided: if (typeof require == "undefined") { require = ... BTW, do we even have the scenario where more than one module bundle is loaded in the same context, or shouldn't we rather avoid this anyway? Then the check whether "require" is already defined would be unnecessary and we could also just use "let" here.
Patch Set 3 : Addressed some feedback https://codereview.adblockplus.org/29399569/diff/29399595/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29399569/diff/29399595/packagerChrome.py#n... packagerChrome.py:145: info_module = None On 2017/03/31 10:42:01, Sebastian Noack wrote: > How about merging chromeInfo.js.tmpl, geckoInfo.js.tmpl and modules.js.tmpl into > one template: > > require.modules["info"] = function(module, exports) > { > {%- if type == "gecko-webext" %} > ... > {%- elif type == "edge" %} > ... > {%- else %} > ... > {%- endif %} Well I tried that and figured that repeating the module wrapping boilreplate for the info module kind of sucked. But then on the other hand perhaps you're right and it's worth it to reduce complexity from the packager. As for splitting out the Edge user-agent parsing code from Chrome's, no thanks. I am not familiar with that code at all, it's hardly trivial and the Edge logic overlaps with the Chrome logic quite a bit. Done... kinda. https://codereview.adblockplus.org/29399569/diff/29399595/templates/chromeInf... File templates/chromeInfo.js.tmpl (right): https://codereview.adblockplus.org/29399569/diff/29399595/templates/chromeInf... templates/chromeInfo.js.tmpl:55: module.exports = { On 2017/03/31 10:42:01, Sebastian Noack wrote: > Replacing module.exports isn't necessary: > > exports.addonName = {{ metadata.get('general', 'basename')|json }}; > exports.addonVersion = {{ version|json }}; > exports.application = application; > exports.applicationVersion = applicationVersion; > exports.platform = platform; > exports.platformVersion = platformVersion; Doing the assignments separately isn't necessary. https://codereview.adblockplus.org/29399569/diff/29399595/templates/modules.j... File templates/modules.js.tmpl (right): https://codereview.adblockplus.org/29399569/diff/29399595/templates/modules.j... templates/modules.js.tmpl:2: if (!("require" in window)) On 2017/03/31 10:42:01, Sebastian Noack wrote: > The assumption of the global object being "window" could be avoided: > > if (typeof require == "undefined") > { > require = ... Done. > BTW, do we even have the scenario where more than one module bundle is loaded in > the same context, or shouldn't we rather avoid this anyway? Then the check > whether "require" is already defined would be unnecessary and we could also just > use "let" here. Yes, we need that for the content scripts. Since at the very minimum there needs to be two content scripts, one for pre and one for post load, but they share context and we want to use modules between them.
https://codereview.adblockplus.org/29399569/diff/29399595/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29399569/diff/29399595/packagerChrome.py#n... packagerChrome.py:145: info_module = None On 2017/03/31 14:03:01, kzar wrote: > On 2017/03/31 10:42:01, Sebastian Noack wrote: > > How about merging chromeInfo.js.tmpl, geckoInfo.js.tmpl and modules.js.tmpl > into > > one template: > > > > require.modules["info"] = function(module, exports) > > { > > {%- if type == "gecko-webext" %} > > ... > > {%- elif type == "edge" %} > > ... > > {%- else %} > > ... > > {%- endif %} > > Well I tried that and figured that repeating the module wrapping boilreplate for > the info module kind of sucked. But then on the other hand perhaps you're right > and it's worth it to reduce complexity from the packager. > > As for splitting out the Edge user-agent parsing code from Chrome's, no thanks. > I am not familiar with that code at all, it's hardly trivial and the Edge logic > overlaps with the Chrome logic quite a bit. > > Done... kinda. We definitely should clean up that mess and target every platform that has separate builds the same way. But I guess, we don't necessarily have to do it with the changes here. https://codereview.adblockplus.org/29399569/diff/29399595/templates/chromeInf... File templates/chromeInfo.js.tmpl (right): https://codereview.adblockplus.org/29399569/diff/29399595/templates/chromeInf... templates/chromeInfo.js.tmpl:55: module.exports = { On 2017/03/31 14:03:01, kzar wrote: > On 2017/03/31 10:42:01, Sebastian Noack wrote: > > Replacing module.exports isn't necessary: > > > > exports.addonName = {{ metadata.get('general', 'basename')|json }}; > > exports.addonVersion = {{ version|json }}; > > exports.application = application; > > exports.applicationVersion = applicationVersion; > > exports.platform = platform; > > exports.platformVersion = platformVersion; > > Doing the assignments separately isn't necessary. Well, this is how we do it for every other module, except (in one or two cases) when there is already a preexisting object whose properties we want to export. Also that way we don't unnecessarily create two objects, discarding the first. https://codereview.adblockplus.org/29399569/diff/29399760/templates/modules.j... File templates/modules.js.tmpl (right): https://codereview.adblockplus.org/29399569/diff/29399760/templates/modules.j... templates/modules.js.tmpl:4: window.require = function(module) This will still assume that there is a global "window" object. But I guess, if we don't use module bundles anywhere but in the web context, that is fine. https://codereview.adblockplus.org/29399569/diff/29399760/templates/modules.j... templates/modules.js.tmpl:17: require.modules["info"] = function(module, exports) The "info" module should only be added for the background script bundle. Perhaps introduce an option/argument (like "module" above) to indicate whether the info module should be added. https://codereview.adblockplus.org/29399569/diff/29399760/templates/modules.j... templates/modules.js.tmpl:20: {% include "geckoInfo.js.tmpl" %} Why not just inline them here? Those templates aren't used anywhere else. Through inlining we can avoid duplication when assigning/returning the exports.
Patch Set 4 : Addressed more feedback https://codereview.adblockplus.org/29399569/diff/29399595/templates/chromeInf... File templates/chromeInfo.js.tmpl (right): https://codereview.adblockplus.org/29399569/diff/29399595/templates/chromeInf... templates/chromeInfo.js.tmpl:55: module.exports = { On 2017/03/31 14:50:45, Sebastian Noack wrote: > On 2017/03/31 14:03:01, kzar wrote: > > On 2017/03/31 10:42:01, Sebastian Noack wrote: > > > Replacing module.exports isn't necessary: > > > > > > exports.addonName = {{ metadata.get('general', 'basename')|json }}; > > > exports.addonVersion = {{ version|json }}; > > > exports.application = application; > > > exports.applicationVersion = applicationVersion; > > > exports.platform = platform; > > > exports.platformVersion = platformVersion; > > > > Doing the assignments separately isn't necessary. > > Well, this is how we do it for every other module, except (in one or two cases) > when there is already a preexisting object whose properties we want to export. > Also that way we don't unnecessarily create two objects, discarding the first. Done. https://codereview.adblockplus.org/29399569/diff/29399760/templates/modules.j... File templates/modules.js.tmpl (right): https://codereview.adblockplus.org/29399569/diff/29399760/templates/modules.j... templates/modules.js.tmpl:4: window.require = function(module) On 2017/03/31 14:50:45, Sebastian Noack wrote: > This will still assume that there is a global "window" object. But I guess, if > we don't use module bundles anywhere but in the web context, that is fine. Perhaps it matters for Firefox, can't harm to fix that anyway. Done. https://codereview.adblockplus.org/29399569/diff/29399760/templates/modules.j... templates/modules.js.tmpl:17: require.modules["info"] = function(module, exports) On 2017/03/31 14:50:45, Sebastian Noack wrote: > The "info" module should only be added for the background script bundle. Perhaps > introduce an option/argument (like "module" above) to indicate whether the info > module should be added. Done. https://codereview.adblockplus.org/29399569/diff/29399760/templates/modules.j... templates/modules.js.tmpl:20: {% include "geckoInfo.js.tmpl" %} On 2017/03/31 14:50:45, Sebastian Noack wrote: > Why not just inline them here? Those templates aren't used anywhere else. > Through inlining we can avoid duplication when assigning/returning the exports. I prefer them as separate files, I think it's easier to read than one huge template.
https://codereview.adblockplus.org/29399569/diff/29399760/templates/modules.j... File templates/modules.js.tmpl (right): https://codereview.adblockplus.org/29399569/diff/29399760/templates/modules.j... templates/modules.js.tmpl:4: window.require = function(module) On 2017/03/31 15:16:54, kzar wrote: > Perhaps it matters for Firefox, can't harm to fix that anyway. (gecko-webext devbuild seems to run OK with these changes, just gave it a quick test.)
https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.j... File templates/modules.js.tmpl (left): https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.j... templates/modules.js.tmpl:1: {%- if args["module"] -%} It seems we can get rid of the "module" argument now. It was only necessary to bundle content scripts by concatenating them instead of generating modules.
Patch Set 5 : Improve check for require https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.j... File templates/modules.js.tmpl (left): https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.j... templates/modules.js.tmpl:1: {%- if args["module"] -%} On 2017/03/31 16:33:11, Sebastian Noack wrote: > It seems we can get rid of the "module" argument now. It was only necessary to > bundle content scripts by concatenating them instead of generating modules. Not quite yet, there are a few scripts that we need to keep bundling for now. For example ext/common.js, ext/background.js and elemHideEmulation.js. I think you're right that we could perhaps remove that in the future though. https://codereview.adblockplus.org/29399569/diff/29399796/templates/modules.j... File templates/modules.js.tmpl (right): https://codereview.adblockplus.org/29399569/diff/29399796/templates/modules.j... templates/modules.js.tmpl:2: if (typeof require != "function") As discussed in IRC this is safer, since if a webpage has an element of id "require" then `require` evaluates to it and so `typeof require == "object"`, at least by the time the postload scripts run.
https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.j... File templates/modules.js.tmpl (left): https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.j... templates/modules.js.tmpl:1: {%- if args["module"] -%} On 2017/04/01 01:32:56, kzar wrote: > On 2017/03/31 16:33:11, Sebastian Noack wrote: > > It seems we can get rid of the "module" argument now. It was only necessary to > > bundle content scripts by concatenating them instead of generating modules. > > Not quite yet, there are a few scripts that we need to keep bundling for now. > For example ext/common.js, ext/background.js and elemHideEmulation.js. I think > you're right that we could perhaps remove that in the future though. It seems elemHideEmulation.js can just be added unchanged as module to the content scripts bundle. We probably should do that anyway. As for ext, since Safari is gone, the separation between ext/common.js and chrome/ext/common.js, and ext/background.js and ext/chrome/background.js doesn't make any sense anymore. These scripts should have been merged when we removed safari/ext. But since this didn't happen, we should do it now. It's quite trivial anyway, and once we did that there is no need to bundle any scripts without the module system anymore. https://codereview.adblockplus.org/29399569/diff/29399796/templates/modules.j... File templates/modules.js.tmpl (right): https://codereview.adblockplus.org/29399569/diff/29399796/templates/modules.j... templates/modules.js.tmpl:2: if (typeof require != "function") On 2017/04/01 01:32:57, kzar wrote: > As discussed in IRC this is safer, since if a webpage has an element of id > "require" then `require` evaluates to it and so `typeof require == "object"`, at > least by the time the postload scripts run. Wow, I never noticed that named elements become automatically available in the global namespace. This is crazy. After reading up on it, I figured that IE first introduced this mess (of course), then other browsers had to follow for compatibility (since back in the days every website targeted IE), and eventually it even made it into the standard: https://www.w3.org/TR/html5/browsers.html#named-access-on-the-window-object
https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.j... File templates/modules.js.tmpl (left): https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.j... templates/modules.js.tmpl:1: {%- if args["module"] -%} On 2017/04/01 10:02:56, Sebastian Noack wrote: > It seems elemHideEmulation.js can just be added unchanged as module to the > content scripts bundle. We probably should do that anyway. > > As for ext, since Safari is gone, the separation between ext/common.js and > chrome/ext/common.js, and ext/background.js and ext/chrome/background.js doesn't > make any sense anymore. These scripts should have been merged when we removed > safari/ext. But since this didn't happen, we should do it now. It's quite > trivial anyway, and once we did that there is no need to bundle any scripts > without the module system anymore. We agree that at some point we can probably remove the module flag, but I don't agree about doing it right now. I've had enough of these refactoring jobs for a while, and hope to focus on the WebRTC work soon. Perhaps this could be a "goodfirstissue" for Jon or one of your mentees?
https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.j... File templates/modules.js.tmpl (left): https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.j... templates/modules.js.tmpl:1: {%- if args["module"] -%} On 2017/04/01 10:50:28, kzar wrote: > On 2017/04/01 10:02:56, Sebastian Noack wrote: > > It seems elemHideEmulation.js can just be added unchanged as module to the > > content scripts bundle. We probably should do that anyway. > > > > As for ext, since Safari is gone, the separation between ext/common.js and > > chrome/ext/common.js, and ext/background.js and ext/chrome/background.js > doesn't > > make any sense anymore. These scripts should have been merged when we removed > > safari/ext. But since this didn't happen, we should do it now. It's quite > > trivial anyway, and once we did that there is no need to bundle any scripts > > without the module system anymore. > > We agree that at some point we can probably remove the module flag, but I don't > agree about doing it right now. I've had enough of these refactoring jobs for a > while, and hope to focus on the WebRTC work soon. Perhaps this could be a > "goodfirstissue" for Jon or one of your mentees? Merging those scripts, while on it, is trivial. If we do it later, however, we need to update buildtools, update the buildtools dependency in adblockpluschrome, and adapt adblockpluschrome, again, which is some effort. Also in regard to that potential no cosmetic changes policy, you voted for, the idea was to clean up code while changing it anyway, like we'd do here. Not saying that we will go with that policy, but it seems inconsistent that you agree to it, but then suggest doing the related clean up separately here.
https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.j... File templates/modules.js.tmpl (left): https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.j... templates/modules.js.tmpl:1: {%- if args["module"] -%} On 2017/04/01 11:04:54, Sebastian Noack wrote: > Merging those scripts, while on it, is trivial. If we do it later, however, we > need to update buildtools, update the buildtools dependency in > adblockpluschrome, and adapt adblockpluschrome, again, which is some effort. > > Also in regard to that potential no cosmetic changes policy, you voted for, the > idea was to clean up code while changing it anyway, like we'd do here. Not > saying that we will go with that policy, but it seems inconsistent that you > agree to it, but then suggest doing the related clean up separately here. Honestly I've have had enough of these "while at it" refactoring requests for a while, especially when each one leads to three more. The ESLint stuff was painful and it left me fairly burnt out there. I plan to work on the WebRTC wrapper next week. I'm happy to try and finish this, or to let someone else take over, but I don't want the scope of the changes to continue to grow if I'm the one stuck making them.
LGTM https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.j... File templates/modules.js.tmpl (left): https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.j... templates/modules.js.tmpl:1: {%- if args["module"] -%} On 2017/04/01 12:13:13, kzar wrote: > On 2017/04/01 11:04:54, Sebastian Noack wrote: > > Merging those scripts, while on it, is trivial. If we do it later, however, we > > need to update buildtools, update the buildtools dependency in > > adblockpluschrome, and adapt adblockpluschrome, again, which is some effort. > > > > Also in regard to that potential no cosmetic changes policy, you voted for, > the > > idea was to clean up code while changing it anyway, like we'd do here. Not > > saying that we will go with that policy, but it seems inconsistent that you > > agree to it, but then suggest doing the related clean up separately here. > > Honestly I've have had enough of these "while at it" refactoring requests for a > while, especially when each one leads to three more. The ESLint stuff was > painful and it left me fairly burnt out there. I plan to work on the WebRTC > wrapper next week. I'm happy to try and finish this, or to let someone else take > over, but I don't want the scope of the changes to continue to grow if I'm the > one stuck making them. Well, you were the one pushing for the ESLint configuration and the related refactoring. Refactoring the content scripts into modules was necessary, in order to use the no-undef rule there (which wasn't even my idea) in a sane way, besides we planned to put everything into modules for a long time anyway. Also you were the one pushing for dropping Safari support, and despite we are talking about a change that was just forgotten there, you turn down every opportunity to correct that. While it was good we did these things, and I'm happy you moved them forward, I think your reaction here isn't fair. Generally, I think, it is important while we advance our code, to watch out for things that can be simplified or cleaned up. Otherwise, we end up in an unmaintainable mess in the long run (if we aren't there yet), whether the code is linted or not. Anyway, I guess, I will file a separate issue. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
