Issue 29549786: Issue 5535 - Replace our module system with webpack (Closed)
|
Created:
Sept. 19, 2017, 2:23 p.m. by kzar Modified:
Oct. 10, 2017, 6:08 p.m. CC:
Thomas Greiner, tlucas Visibility:
Public. |
DescriptionIssue 5535 - Replace our module system with webpack
I've got a rough version of the corresponding adblockpluschrome changes working (minus the popup), which might help give some context to this review.
https://github.com/adblockplus/adblockpluschrome/compare/master...kzar:5080-modules?expand=1
If you want to try the new version which avoids writing files to the disk, you'll need to use this branch instead:
https://github.com/adblockplus/adblockpluschrome/compare/master...kzar:5080-modules-memory?expand=1
Patch Set 1 #Patch Set 2 : Fixed spelling mistake #
Total comments: 2
Patch Set 3 : Silence webpack output except errors and warnings #Patch Set 4 : Delete all temporary files at the same time #
Total comments: 9
Patch Set 5 : Addressed Sebastian's feedback #Patch Set 6 : Hardcode resolve paths #Patch Set 7 : Improved resolve paths comment #
Total comments: 4
Patch Set 8 : Removed ext_background workaround #
Total comments: 7
Patch Set 9 : Use absolute paths for entry points #Patch Set 10 : Avoid writing files when producing webpack bundles #
Total comments: 2
Patch Set 11 : Only use special info-loader for adblockpluschrome/lib/info.js #Patch Set 12 : Expose stats Object #
Total comments: 19
Patch Set 13 : Improve output on error #Patch Set 14 : Addressed Wladimir's feedback #Patch Set 15 : Tidy up JSON passed from packagerChrome.py #Patch Set 16 : Produce all bundles in one Node.js process to speed things up #Patch Set 17 : Avoid creating multiple webpack compilers #Patch Set 18 : Fixed tox errors #
Total comments: 8
Patch Set 19 : Addressed Wladimir's feedback, use JSON and standard in + out #Patch Set 20 : Rebased and tidied up #
Total comments: 7
Patch Set 21 : Addressed final nits #
MessagesTotal messages: 38
Patch Set 1
Patch Set 2 : Fixed spelling mistake
Hey Dave! The context you provided helped a lot (got it running in as much as an instant). So far, i only have one comment, please see below. Cheers, Tristan https://codereview.adblockplus.org/29549786/diff/29549795/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29549786/diff/29549795/packagerChrome.py#n... packagerChrome.py:161: with tempfile.NamedTemporaryFile(delete=False) as info_file: You are creating two separate temporary instances - resulting in having to take care of getting rid of two things i two different ways. What do think about about just using the temporary folder, along with a subfolder 'files' and a file 'info_module' (which you write the rendered template to)? That way you would only have to call finally: shutil.rmtree(temp_dir) in order to get rid of everything temporary.
Patch Set 3 : Silence webpack output except errors and warnings Patch Set 4 : Delete all temporary files at the same time https://codereview.adblockplus.org/29549786/diff/29549795/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29549786/diff/29549795/packagerChrome.py#n... packagerChrome.py:161: with tempfile.NamedTemporaryFile(delete=False) as info_file: On 2017/09/22 10:27:07, tlucas wrote: > You are creating two separate temporary instances - resulting in having to take > care of getting rid of two things i two different ways. > > What do think about about just using the temporary folder, along with a > subfolder 'files' and a file 'info_module' (which you write the rendered > template to)? > > That way you would only have to call > > finally: > shutil.rmtree(temp_dir) > > in order to get rid of everything temporary. Good idea, we might as well have everything deleted at the same time. Done.
https://codereview.adblockplus.org/29549786/diff/29552694/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29549786/diff/29552694/packagerChrome.py#n... packagerChrome.py:157: './adblockplusui/lib']) Mind filing a follow up issue (and referring to it here), to replace absolute paths across adblockpluschrome/ablockplusui/andblockpluscore? For the time being, I'd prefer to look in 'lib' + '*/lib', rather than hard-coding these paths. This however, would not work because 'buildtools/lib' which includes conflicting scripts, which are only used in legacy Gecko extensions. But we should remove support for legacy Gecko builds (as well as for Safari builds) from buildtools, anyway, regardless of this change. Can you file an issue for that as well? https://codereview.adblockplus.org/29549786/diff/29552694/packagerChrome.py#n... packagerChrome.py:159: temp_dir = tempfile.mkdtemp() This should be outside of the try-finally block. Otherwise, if it fails, the code in the finally block would fail as well. https://codereview.adblockplus.org/29549786/diff/29552694/packagerChrome.py#n... packagerChrome.py:161: with tempfile.NamedTemporaryFile(delete=False, Do we need to use NamedTemporaryFile here, or can't we just hard-code the filename, since we are inside a temporary folder anyway?
https://codereview.adblockplus.org/29549786/diff/29552694/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29549786/diff/29552694/packagerChrome.py#n... packagerChrome.py:157: './adblockplusui/lib']) On 2017/09/22 19:28:02, Sebastian Noack wrote: > But we should remove support for legacy Gecko builds (as well as for > Safari builds) from buildtools, anyway, regardless of this change. Can you > file an issue for that as well? Never mind, these issues have already been filed, apparently: https://issues.adblockplus.org/ticket/5751 https://issues.adblockplus.org/ticket/5752
Patch Set 5 : Addressed Sebastian's feedback https://codereview.adblockplus.org/29549786/diff/29552694/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29549786/diff/29552694/packagerChrome.py#n... packagerChrome.py:157: './adblockplusui/lib']) On 2017/09/22 19:28:02, Sebastian Noack wrote: > Mind filing a follow up issue (and referring to it here), to replace absolute > paths across adblockpluschrome/ablockplusui/andblockpluscore? > > For the time being, I'd prefer to look in 'lib' + '*/lib', rather than > hard-coding these paths. This however, would not work because 'buildtools/lib' > which includes conflicting scripts, which are only used in legacy Gecko > extensions. But we should remove support for legacy Gecko builds (as well as for > Safari builds) from buildtools, anyway, regardless of this change. Can you file > an issue for that as well? OK since we're now considering removing buildtools/lib a blocker for this issue I figured we could do that. I ran into two problems though. Firstly devenv.*/lib directories were included, which broke everything. So I reused the ensure_dependencies logic to only check directories which match our dependencies. Second problem I hit was that elemHideEmulation started resolving to adblockplus/lib/elemHideEmulation.js which broke everything too. So now the logic says check the lib directory of all our immediate dependencies unless they're called "adblockplus". I do wonder if it's worth the trouble however, perhaps hardcoding them was better after all? https://codereview.adblockplus.org/29549786/diff/29552694/packagerChrome.py#n... packagerChrome.py:159: temp_dir = tempfile.mkdtemp() On 2017/09/22 19:28:02, Sebastian Noack wrote: > This should be outside of the try-finally block. Otherwise, if it fails, the > code in the finally block would fail as well. Done. https://codereview.adblockplus.org/29549786/diff/29552694/packagerChrome.py#n... packagerChrome.py:161: with tempfile.NamedTemporaryFile(delete=False, On 2017/09/22 19:28:02, Sebastian Noack wrote: > Do we need to use NamedTemporaryFile here, or can't we just hard-code the > filename, since we are inside a temporary folder anyway? Done. (But I wonder if the file name is more likely to clash now.)
https://codereview.adblockplus.org/29549786/diff/29552694/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29549786/diff/29552694/packagerChrome.py#n... packagerChrome.py:157: './adblockplusui/lib']) On 2017/09/23 20:26:36, kzar wrote: > On 2017/09/22 19:28:02, Sebastian Noack wrote: > > Mind filing a follow up issue (and referring to it here), to replace absolute > > paths across adblockpluschrome/ablockplusui/andblockpluscore? It seems you missed this part? > > For the time being, I'd prefer to look in 'lib' + '*/lib', rather than > > hard-coding these paths. This however, would not work because 'buildtools/lib' > > which includes conflicting scripts, which are only used in legacy Gecko > > extensions. But we should remove support for legacy Gecko builds (as well as > for > > Safari builds) from buildtools, anyway, regardless of this change. Can you > file > > an issue for that as well? > > OK since we're now considering removing buildtools/lib a blocker for this issue > I figured we could do that. I ran into two problems though. Firstly devenv.*/lib > directories were included, which broke everything. So I reused the > ensure_dependencies logic to only check directories which match our > dependencies. Second problem I hit was that elemHideEmulation started resolving > to adblockplus/lib/elemHideEmulation.js which broke everything too. > > So now the logic says check the lib directory of all our immediate dependencies > unless they're called "adblockplus". I do wonder if it's worth the trouble > however, perhaps hardcoding them was better after all? Dang. FWIW, we also want to get rid of the "adblockplus" dependency. This repository is deprecated now, and even before, since "adblockpluscore" moved to a separate repository, we only kept the dependency on "adblockplus" as temporary hack to import strings until we moved them where they belong. There should be an issue for that, otherwise we should file one! However, since the issue tracker is currently down, I cannot look into it. Anyway, I do not necessarily want to block this review by an endless lists of cleanups that are overdue, in particular since the code here is a temporary workaround anyway. Feel free to go back to your initial approach just whitelsiting the relevant paths, for now, and remove the dependency on #5751. (Still I would love to see, #5751 and #5752 move forward independently.)
Patch Set 6 : Hardcode resolve paths https://codereview.adblockplus.org/29549786/diff/29552694/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29549786/diff/29552694/packagerChrome.py#n... packagerChrome.py:157: './adblockplusui/lib']) On 2017/09/23 20:49:01, Sebastian Noack wrote: > On 2017/09/23 20:26:36, kzar wrote: > > On 2017/09/22 19:28:02, Sebastian Noack wrote: > > > Mind filing a follow up issue (and referring to it here), to replace > absolute > > > paths across adblockpluschrome/ablockplusui/andblockpluscore? > > It seems you missed this part? Oh, I forgot to mention that I tried to file the issues but the tracker was down. I've done that now see #5760, #5761 and #5762. > Anyway, I do not necessarily want to block this review by an endless > lists of cleanups that are overdue, in particular since the code here > is a temporary workaround anyway. Feel free to go back to your initial > approach just whitelsiting the relevant paths, for now, and remove the > dependency on #5751. (Still I would love to see, #5751 and #5752 move > forward independently.) Yea I agree, Done.
Patch Set 7 : Improved resolve paths comment
https://codereview.adblockplus.org/29549786/diff/29554571/webpack.config.js File webpack.config.js (right): https://codereview.adblockplus.org/29549786/diff/29554571/webpack.config.js#n... webpack.config.js:32: ext_background: "ext" adblockplusui attempts to require ext_background only for the legacy Gecko extension. This require should be removed now. There is no need to turn ext into a module for the WebExtension, we should rather get rid of it eventually.
https://codereview.adblockplus.org/29549786/diff/29554571/webpack.config.js File webpack.config.js (right): https://codereview.adblockplus.org/29549786/diff/29554571/webpack.config.js#n... webpack.config.js:32: ext_background: "ext" On 2017/09/24 15:03:38, Sebastian Noack wrote: > adblockplusui attempts to require ext_background only for the > legacy Gecko extension. This require should be removed now. There > is no need to turn ext into a module for the WebExtension, we > should rather get rid of it eventually. FWIW This workaround simply avoids webpack replacing require("ext_background") with anything, which is required since `ext` is falsey at bundle time but not at runtime for the extension[1]. It doesn't turn ext into a module. But of course I agree, if we can remove the require and this workaround then even better. [1] For reference in case someone else's reading through this discussion the line in adblockplusui/messageResponder.js looks like `let ext = global.ext || require("ext_background");`.
(Adding Thomas to Cc since we've been discussing the require("ext_background")
in adblockplusui/messageResponder.js.)
https://codereview.adblockplus.org/29549786/diff/29554571/webpack.config.js File webpack.config.js (right): https://codereview.adblockplus.org/29549786/diff/29554571/webpack.config.js#n... webpack.config.js:32: ext_background: "ext" On 2017/09/24 16:02:46, kzar wrote: > On 2017/09/24 15:03:38, Sebastian Noack wrote: > > adblockplusui attempts to require ext_background only for the > > legacy Gecko extension. This require should be removed now. There > > is no need to turn ext into a module for the WebExtension, we > > should rather get rid of it eventually. > > FWIW This workaround simply avoids webpack replacing require("ext_background") > with anything, which is required since `ext` is falsey at bundle time but not at > runtime for the extension[1]. It doesn't turn ext into a module. > > But of course I agree, if we can remove the require and this workaround then > even better. > > [1] For reference in case someone else's reading through this discussion the > line in adblockplusui/messageResponder.js looks like `let ext = global.ext || > require("ext_background");`. You already removed other code from adblockplusui that was necessary for the legacy Gecko extension to work, but had to be removed for Webpack: https://hg.adblockplus.org/adblockplusui/rev/2fb191f6a20c This is the same with require("ext_background"), no reason to go with a workaround, this time, IMO.
Patch Set 8 : Removed ext_background workaround https://codereview.adblockplus.org/29549786/diff/29554571/webpack.config.js File webpack.config.js (right): https://codereview.adblockplus.org/29549786/diff/29554571/webpack.config.js#n... webpack.config.js:32: ext_background: "ext" On 2017/09/24 20:42:15, Sebastian Noack wrote: > On 2017/09/24 16:02:46, kzar wrote: > > On 2017/09/24 15:03:38, Sebastian Noack wrote: > > > adblockplusui attempts to require ext_background only for the > > > legacy Gecko extension. This require should be removed now. There > > > is no need to turn ext into a module for the WebExtension, we > > > should rather get rid of it eventually. > > > > FWIW This workaround simply avoids webpack replacing require("ext_background") > > with anything, which is required since `ext` is falsey at bundle time but not > at > > runtime for the extension[1]. It doesn't turn ext into a module. > > > > But of course I agree, if we can remove the require and this workaround then > > even better. > > > > [1] For reference in case someone else's reading through this discussion the > > line in adblockplusui/messageResponder.js looks like `let ext = global.ext || > > require("ext_background");`. > > You already removed other code from adblockplusui that was necessary for the > legacy Gecko extension to work, but had to be removed for Webpack: > https://hg.adblockplus.org/adblockplusui/rev/2fb191f6a20c > > This is the same with require("ext_background"), no reason to go with a > workaround, this time, IMO. Done. (awaiting https://codereview.adblockplus.org/29555744/)
LGTM
LGTM
https://codereview.adblockplus.org/29549786/diff/29555770/package.json File package.json (right): https://codereview.adblockplus.org/29549786/diff/29555770/package.json#newcode11 package.json:11: "webpack": "webpack" It's probably better to transform the webpack.config.js into a webpack_runner.js and call "node webpack_runner.js" here. Then we'll be able to create a webpack compiler instance explicitly and make sure it writes to memory rather than the file system, forwarding the results to stdout so that the Python code can get them. https://codereview.adblockplus.org/29549786/diff/29555770/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29549786/diff/29555770/packagerChrome.py#n... packagerChrome.py:173: ) Writing a temporary file here should be avoidable if we define a custom loader for it (see https://webpack.js.org/concepts/loaders/). https://codereview.adblockplus.org/29549786/diff/29555770/packagerChrome.py#n... packagerChrome.py:185: base_extension_path) Please use absolute paths here, this should make things much more straightforward. https://codereview.adblockplus.org/29549786/diff/29555770/packagerChrome.py#n... packagerChrome.py:201: ) Writing to temporary files shouldn't be necessary, see https://webpack.js.org/api/node/#custom-file-systems on how to compile bundles in memory. This implies calling the webpack module directly rather than using the CLI however. https://codereview.adblockplus.org/29549786/diff/29555770/webpack.config.js File webpack.config.js (right): https://codereview.adblockplus.org/29549786/diff/29555770/webpack.config.js#n... webpack.config.js:25: devtool: "source-map", We might want to use "inline-source-map" here, will make things simpler because we have only one output file per bundle.
Patch Set 9 : Use absolute paths for entry points Patch Set 10 : Avoid writing files when producing webpack bundles While I've gotten this working without writing files now I wonder if it's worth the trouble. I'm curious what everyone thinks. One problem I've found with this approach is it's quite a bit slower. I've just had a quick test to see how long the devbuild takes: Before my changes: ~0.7s Using webpack, writing to files: ~3.2s Using weback, in memory: ~4.7s On the positive side source maps, console error output and the extension are still all working as before. https://codereview.adblockplus.org/29549786/diff/29555770/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29549786/diff/29555770/packagerChrome.py#n... packagerChrome.py:185: base_extension_path) On 2017/09/26 12:06:13, Wladimir Palant wrote: > Please use absolute paths here, this should make things much more > straightforward. Good idea, Done. (I tried to do the same with bundle_file while at it but webpack insisted on a relative path there.) https://codereview.adblockplus.org/29549786/diff/29555770/webpack.config.js File webpack.config.js (right): https://codereview.adblockplus.org/29549786/diff/29555770/webpack.config.js#n... webpack.config.js:25: devtool: "source-map", On 2017/09/26 12:06:14, Wladimir Palant wrote: > We might want to use "inline-source-map" here, will make things simpler because > we have only one output file per bundle. It looks like the devtool setting I was using previously was the source of my eval problems. When I changed to use "source-map" things started behaving! Done. https://codereview.adblockplus.org/29549786/diff/29562642/info-loader.js File info-loader.js (right): https://codereview.adblockplus.org/29549786/diff/29562642/info-loader.js#newc... info-loader.js:18: module.exports = source => require("process").env.INFO_MODULE; While this does work, it also requires the lib/info.js file to exist. So I've created that file but just put a comment in there. There might be a way around that[1] but it seems more trouble than it's worth. [1] - https://github.com/rmarscher/virtual-module-webpack-plugin https://codereview.adblockplus.org/29549786/diff/29562642/package.json File package.json (left): https://codereview.adblockplus.org/29549786/diff/29562642/package.json#oldcode11 package.json:11: "webpack": "webpack" I found that `npm run-script` didn't pass my environment variables through, so instead I now call node directly from the Python code.
Patch Set 11 : Only use special info-loader for adblockpluschrome/lib/info.js
Dave, than you for improving the approach here, this is looking much better already. Some comments below. https://codereview.adblockplus.org/29549786/diff/29563589/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29549786/diff/29563589/packagerChrome.py#n... packagerChrome.py:160: template = getTemplate(info_templates[params['type']]) Nit: this variable is better named info_template I guess. https://codereview.adblockplus.org/29549786/diff/29563589/packagerChrome.py#n... packagerChrome.py:176: cwd=os.path.dirname(__file__), I'd prefer an absolute path to webpack_runner.js instead of the cwd parameter. https://codereview.adblockplus.org/29549786/diff/29563589/packagerChrome.py#n... packagerChrome.py:179: 'ENTRY_POINTS': ' '.join(entry_files), This will break for paths containing spaces. https://codereview.adblockplus.org/29549786/diff/29563589/packagerChrome.py#n... packagerChrome.py:182: 'INFO_MODULE': info_module, Why do we still need to pass data via environment variables? Command line parameters should make more sense, or maybe even writing a JSON-encoded blob to stdin - this should be safer to parse. https://codereview.adblockplus.org/29549786/diff/29563589/webpack_runner.js File webpack_runner.js (right): https://codereview.adblockplus.org/29549786/diff/29563589/webpack_runner.js#n... webpack_runner.js:69: include: path.join(EXTENSION_PATH, "lib", "info.js"), This looks like we are supposed to do require("./info"). That would be inconsistent however, with the file info.js not actually existing in the directory. It would make more sense if require("info") would work, this is really a platform module. If `test: /^info$/` won't do the job, then a resolve alias should do. The alias should also make the rule here unnecessary, you can specify "info-loader!info.json" as explicit alias target. https://codereview.adblockplus.org/29549786/diff/29563589/webpack_runner.js#n... webpack_runner.js:127: e => I prefer a separate .catch() callback, it makes code more obvious - particularly with multi-line callbacks. https://codereview.adblockplus.org/29549786/diff/29563589/webpack_runner.js#n... webpack_runner.js:134: // will terminate the Node.js process with a non-zero exit code." Well, Node correctly objects to unhandled rejections, is it really worth stating here?
Patch Set 14 : Addressed Wladimir's feedback https://codereview.adblockplus.org/29549786/diff/29563589/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29549786/diff/29563589/packagerChrome.py#n... packagerChrome.py:160: template = getTemplate(info_templates[params['type']]) On 2017/10/04 10:38:06, Wladimir Palant wrote: > Nit: this variable is better named info_template I guess. Done. https://codereview.adblockplus.org/29549786/diff/29563589/packagerChrome.py#n... packagerChrome.py:176: cwd=os.path.dirname(__file__), On 2017/10/04 10:38:06, Wladimir Palant wrote: > I'd prefer an absolute path to webpack_runner.js instead of the cwd parameter. Done. https://codereview.adblockplus.org/29549786/diff/29563589/packagerChrome.py#n... packagerChrome.py:179: 'ENTRY_POINTS': ' '.join(entry_files), On 2017/10/04 10:38:06, Wladimir Palant wrote: > This will break for paths containing spaces. Whoops, good point. https://codereview.adblockplus.org/29549786/diff/29563589/packagerChrome.py#n... packagerChrome.py:182: 'INFO_MODULE': info_module, On 2017/10/04 10:38:06, Wladimir Palant wrote: > Why do we still need to pass data via environment variables? Command line > parameters should make more sense, or maybe even writing a JSON-encoded blob to > stdin - this should be safer to parse. Done. https://codereview.adblockplus.org/29549786/diff/29563589/webpack_runner.js File webpack_runner.js (right): https://codereview.adblockplus.org/29549786/diff/29563589/webpack_runner.js#n... webpack_runner.js:69: include: path.join(EXTENSION_PATH, "lib", "info.js"), On 2017/10/04 10:38:07, Wladimir Palant wrote: > This looks like we are supposed to do require("./info"). That would be > inconsistent however, with the file info.js not actually existing in the > directory. It would make more sense if require("info") would work, this is > really a platform module. If `test: /^info$/` won't do the job, then a resolve > alias should do. The alias should also make the rule here unnecessary, you can > specify "info-loader!info.json" as explicit alias target. I originally tried `test: /^info$/` but it turned out the resolved absolute path for the module was used. Then I did something like `test: /info.js$` but I realised that would also match other modules which were called (or had a name ending in) info.js. So in the end I went with the absolute path, anything which webpack resolved to the placeholder file would be replaced, but nothing else. I've tried using an alias like you suggested instead but I can't get it to work, when using an absolute or relative path. https://codereview.adblockplus.org/29549786/diff/29563589/webpack_runner.js#n... webpack_runner.js:127: e => On 2017/10/04 10:38:07, Wladimir Palant wrote: > I prefer a separate .catch() callback, it makes code more obvious - particularly > with multi-line callbacks. Done. https://codereview.adblockplus.org/29549786/diff/29563589/webpack_runner.js#n... webpack_runner.js:134: // will terminate the Node.js process with a non-zero exit code." On 2017/10/04 10:38:07, Wladimir Palant wrote: > Well, Node correctly objects to unhandled rejections, is it really worth stating > here? Done.
Patch Set 15 : Tidy up JSON passed from packagerChrome.py
On 2017/10/02 18:48:33, kzar wrote: > One problem I've found with this approach is it's quite a bit slower. I've just > had a quick test to see how long the devbuild takes: > > Before my changes: ~0.7s > Using webpack, writing to files: ~3.2s > Using weback, in memory: ~4.7s So I've been looking into this. It appears to be the overhead of Node.js starting and loading all the modules we're using. I've tried modifying webpack_runner.js to call webpackInMemory three times instead of once and it barely increases the run time at all! So I'm now trying to figure out if we can avoid starting Node.js multiple times. Perhaps we can accept multiple configurations to STDIN, and somehow delimit the bundles in the output.
Patch Set 16 : Produce all bundles in one Node.js process to speed things up With some quick testing I found this sped the build time from about 4.8 seconds to about 2.2 seconds. It also will mean the build time won't grow linearly as we add more bundles.
Patch Set 17 : Avoid creating multiple webpack compilers
Hey there! I'm moving myself to CC, since the latest / upcoming changes seem to be mostly javascript related. IMHO my minor experience with javascript / node wouldn't add much value to the review. However - i couldn't help having another look at it and things do look good to me (especially the performance-boost from 4.8 to 2.2 seconds) Nicely done! Best, Tristan
Patch Set 18 : Fixed tox errors
https://codereview.adblockplus.org/29549786/diff/29563589/webpack_runner.js File webpack_runner.js (right): https://codereview.adblockplus.org/29549786/diff/29563589/webpack_runner.js#n... webpack_runner.js:69: include: path.join(EXTENSION_PATH, "lib", "info.js"), On 2017/10/04 13:13:25, kzar wrote: > I originally tried `test: /^info$/` but it turned out the resolved absolute path > for the module was used. Then I did something like `test: /info.js$` but I > realised that would also match other modules which were called (or had a name > ending in) info.js. So in the end I went with the absolute path, anything which > webpack resolved to the placeholder file would be replaced, but nothing else. I tried this as well and it seems that module resolution happens before the loader has a chance - so the file has to exist. However, aliases can be used to map it to anything, and mapping it to info-loader.js itself works. Here is the configuration that worked for me: module: { rules: [{ include: path.join(__dirname, "info-loader.js"), use: ["info-loader"] }] }, resolve: { alias: { info$: path.join(__dirname, "info-loader.js"), }, }, resolveLoader: { modules: [path.resolve(__dirname)] } https://codereview.adblockplus.org/29549786/diff/29567764/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29549786/diff/29567764/packagerChrome.py#n... packagerChrome.py:177: 'RESOLVE_PATHS': resolve_paths, Nit: it's a JSON structure, why are properties all-caps? https://codereview.adblockplus.org/29549786/diff/29567764/packagerChrome.py#n... packagerChrome.py:196: toJson(configuration)] Passing the configuration as parameter has an ugly side-effect: if the command fails, the error output is massive due to quoting the command line parameters. Also, the size of command line parameters might be limited on some platforms (Windows?). You could use subprocess.communicate() to pass data to the process via stdin. Note also that info_loader.js doesn't need to parse input separately - webpack_runner.js could require("./info_loader") and call some method to set info module source. https://codereview.adblockplus.org/29549786/diff/29567764/webpack_runner.js File webpack_runner.js (right): https://codereview.adblockplus.org/29549786/diff/29567764/webpack_runner.js#n... webpack_runner.js:29: // To acheive this we output all the required bundles - along with their Typo: achieve https://codereview.adblockplus.org/29549786/diff/29567764/webpack_runner.js#n... webpack_runner.js:125: memoryFS.readFileSync(mappath, "utf-8"), BOUNDARY); How about printing JSON instead of this boundary hack?
Patch Set 19 : Addressed Wladimir's feedback, use JSON and standard in + out https://codereview.adblockplus.org/29549786/diff/29563589/webpack_runner.js File webpack_runner.js (right): https://codereview.adblockplus.org/29549786/diff/29563589/webpack_runner.js#n... webpack_runner.js:69: include: path.join(EXTENSION_PATH, "lib", "info.js"), On 2017/10/09 10:54:54, Wladimir Palant wrote: > On 2017/10/04 13:13:25, kzar wrote: > > I originally tried `test: /^info$/` but it turned out the resolved absolute > path > > for the module was used. Then I did something like `test: /info.js$` but I > > realised that would also match other modules which were called (or had a name > > ending in) info.js. So in the end I went with the absolute path, anything > which > > webpack resolved to the placeholder file would be replaced, but nothing else. > > I tried this as well and it seems that module resolution happens before the > loader has a chance - so the file has to exist. However, aliases can be used to > map it to anything, and mapping it to info-loader.js itself works. Here is the > configuration that worked for me: > > module: { > rules: [{ > include: path.join(__dirname, "info-loader.js"), > use: ["info-loader"] > }] > }, > resolve: { > alias: { > info$: path.join(__dirname, "info-loader.js"), > }, > }, > resolveLoader: { > modules: [path.resolve(__dirname)] > } Cool idea, Done. https://codereview.adblockplus.org/29549786/diff/29567764/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29549786/diff/29567764/packagerChrome.py#n... packagerChrome.py:177: 'RESOLVE_PATHS': resolve_paths, On 2017/10/09 10:54:54, Wladimir Palant wrote: > Nit: it's a JSON structure, why are properties all-caps? Done. https://codereview.adblockplus.org/29549786/diff/29567764/packagerChrome.py#n... packagerChrome.py:196: toJson(configuration)] On 2017/10/09 10:54:55, Wladimir Palant wrote: > Passing the configuration as parameter has an ugly side-effect: if the command > fails, the error output is massive due to quoting the command line parameters. > Also, the size of command line parameters might be limited on some platforms > (Windows?). You could use subprocess.communicate() to pass data to the process > via stdin. Note also that info_loader.js doesn't need to parse input separately > - webpack_runner.js could require("./info_loader") and call some method to set > info module source. Done, you're right the error output looks way nicer now. (We could also catch the subprocess.CalledProcessError, and re-throw with the configuration removed from the command I suppose. That would be simpler but seems like kind of a hack.) https://codereview.adblockplus.org/29549786/diff/29567764/webpack_runner.js File webpack_runner.js (right): https://codereview.adblockplus.org/29549786/diff/29567764/webpack_runner.js#n... webpack_runner.js:29: // To acheive this we output all the required bundles - along with their On 2017/10/09 10:54:55, Wladimir Palant wrote: > Typo: achieve Done. https://codereview.adblockplus.org/29549786/diff/29567764/webpack_runner.js#n... webpack_runner.js:125: memoryFS.readFileSync(mappath, "utf-8"), BOUNDARY); On 2017/10/09 10:54:56, Wladimir Palant wrote: > How about printing JSON instead of this boundary hack? Done.
Patch Set 20 : Rebased and tidied up Some of the inter-diffs aren't working correctly, I must have done something wrong. If it helps you can view my changes in GitHub https://github.com/adblockplus/buildtools/compare/master...kzar:5535-webpack-...
https://codereview.adblockplus.org/29549786/diff/29563589/webpack_runner.js File webpack_runner.js (right): https://codereview.adblockplus.org/29549786/diff/29563589/webpack_runner.js#n... webpack_runner.js:69: include: path.join(EXTENSION_PATH, "lib", "info.js"), On 2017/10/09 13:52:14, kzar wrote: > On 2017/10/09 10:54:54, Wladimir Palant wrote: > > On 2017/10/04 13:13:25, kzar wrote: > > > I originally tried `test: /^info$/` but it turned out the resolved absolute > > path > > > for the module was used. Then I did something like `test: /info.js$` but I > > > realised that would also match other modules which were called (or had a > name > > > ending in) info.js. So in the end I went with the absolute path, anything > > which > > > webpack resolved to the placeholder file would be replaced, but nothing > else. > > > > I tried this as well and it seems that module resolution happens before the > > loader has a chance - so the file has to exist. However, aliases can be used > to > > map it to anything, and mapping it to info-loader.js itself works. Here is the > > configuration that worked for me: > > > > module: { > > rules: [{ > > include: path.join(__dirname, "info-loader.js"), > > use: ["info-loader"] > > }] > > }, > > resolve: { > > alias: { > > info$: path.join(__dirname, "info-loader.js"), > > }, > > }, > > resolveLoader: { > > modules: [path.resolve(__dirname)] > > } > > Cool idea, Done. I've just noticed a down-side to this approach, it messes up the source maps. The info module is listed under buildtools/lib/info-loader.js instead of lib/info.js.
LGTM The nit below is informational only, I don't think that it is important enough to create another patchset. https://codereview.adblockplus.org/29549786/diff/29563589/webpack_runner.js File webpack_runner.js (right): https://codereview.adblockplus.org/29549786/diff/29563589/webpack_runner.js#n... webpack_runner.js:69: include: path.join(EXTENSION_PATH, "lib", "info.js"), On 2017/10/10 13:51:04, kzar wrote: > I've just noticed a down-side to this approach, it messes up the source maps. > The info module is listed under buildtools/lib/info-loader.js instead of > lib/info.js. Well, if we really want it to show up as info.js we could add a zero-sized info.js file to buildtools. Not sure it's worth it however. https://codereview.adblockplus.org/29549786/diff/29572706/webpack_runner.js File webpack_runner.js (right): https://codereview.adblockplus.org/29549786/diff/29572706/webpack_runner.js#n... webpack_runner.js:146: memoryFS.unlinkSync(mappath); Nit: Removing files in a virtual file system isn't really worth doing.
https://codereview.adblockplus.org/29549786/diff/29572706/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29549786/diff/29572706/packagerChrome.py#n... packagerChrome.py:163: for dir in ['', 'adblockpluscore', 'adblockplusui']] The "adblockplus" dependency has been removed meanwhile, and "buildtools/lib" is gone too. So we don't have to hard-code adblockpluscore and adblockplusui here anymore. https://codereview.adblockplus.org/29549786/diff/29572706/packagerChrome.py#n... packagerChrome.py:192: os.path.join(os.path.dirname(__file__), 'webpack_runner.js')] Nit: It seem you don't need to wrap this list if you ge rid of the temporary variable: process = subprocess.Popen( ['node', os.path.join(os.path.dirname(__file__), 'webpack_runner.js')] command, stdout=subprocess.PIPE, stdin=subprocess.PIPE )
Patch Set 21 : Addressed final nits https://codereview.adblockplus.org/29549786/diff/29563589/webpack_runner.js File webpack_runner.js (right): https://codereview.adblockplus.org/29549786/diff/29563589/webpack_runner.js#n... webpack_runner.js:69: include: path.join(EXTENSION_PATH, "lib", "info.js"), On 2017/10/10 16:33:15, Wladimir Palant wrote: > On 2017/10/10 13:51:04, kzar wrote: > > I've just noticed a down-side to this approach, it messes up the source maps. > > The info module is listed under buildtools/lib/info-loader.js instead of > > lib/info.js. > > Well, if we really want it to show up as info.js we could add a zero-sized > info.js file to buildtools. Not sure it's worth it however. Done. https://codereview.adblockplus.org/29549786/diff/29572706/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29549786/diff/29572706/packagerChrome.py#n... packagerChrome.py:163: for dir in ['', 'adblockpluscore', 'adblockplusui']] On 2017/10/10 16:35:19, Sebastian Noack wrote: > The "adblockplus" dependency has been removed meanwhile, and "buildtools/lib" is > gone too. So we don't have to hard-code adblockpluscore and adblockplusui here > anymore. This is true but as discussed in IRC we can remove the resolve_paths and most aliases after the relative paths have been removed anyway, so we might as well keep it simple for now. https://codereview.adblockplus.org/29549786/diff/29572706/packagerChrome.py#n... packagerChrome.py:192: os.path.join(os.path.dirname(__file__), 'webpack_runner.js')] On 2017/10/10 16:35:19, Sebastian Noack wrote: > Nit: It seem you don't need to wrap this list if you ge rid of the temporary > variable: > > process = subprocess.Popen( > ['node', os.path.join(os.path.dirname(__file__), 'webpack_runner.js')] > command, stdout=subprocess.PIPE, stdin=subprocess.PIPE > ) Well I pass the variable to the CalledProcessError, but how about this? https://codereview.adblockplus.org/29549786/diff/29572706/webpack_runner.js File webpack_runner.js (right): https://codereview.adblockplus.org/29549786/diff/29572706/webpack_runner.js#n... webpack_runner.js:146: memoryFS.unlinkSync(mappath); On 2017/10/10 16:33:16, Wladimir Palant wrote: > Nit: Removing files in a virtual file system isn't really worth doing. Done.
LGTM https://codereview.adblockplus.org/29549786/diff/29572706/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29549786/diff/29572706/packagerChrome.py#n... packagerChrome.py:192: os.path.join(os.path.dirname(__file__), 'webpack_runner.js')] On 2017/10/10 17:03:39, kzar wrote: > On 2017/10/10 16:35:19, Sebastian Noack wrote: > > Nit: It seem you don't need to wrap this list if you ge rid of the temporary > > variable: > > > > process = subprocess.Popen( > > ['node', os.path.join(os.path.dirname(__file__), 'webpack_runner.js')] > > command, stdout=subprocess.PIPE, stdin=subprocess.PIPE > > ) > > Well I pass the variable to the CalledProcessError, but how about this? Acknowledged.
Message was sent while issue was closed.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
