Issue 29710555: Issue 6426 - Start using CSS modularization tool in adblockplusui (Closed)
|
Created:
Feb. 27, 2018, 11:37 a.m. by a.giammarchi Modified:
March 7, 2018, 1:57 p.m. Visibility:
Public. |
DescriptionIssue 6426 - Start using CSS modularization tool in adblockplusui
Patch Set 1 #
Total comments: 23
MessagesTotal messages: 15
https://codereview.adblockplus.org/29710555/diff/29710556/package.json File package.json (right): https://codereview.adblockplus.org/29710555/diff/29710556/package.json#newcode11 package.json:11: "bundle:desktop-options": "eslint ./js/**/*.js && echo '/* eslint-disable */// BUNDLED FILE'>./desktop-options.js && browserify --node --no-bundle-external -t [ browserify-css --minify=true --output skin/desktop-options.css ] js/desktop-options.js >> ./desktop-options.js", This throws and error for me: Error: Cannot find module '../css/desktop-options.css' from '/home/saroyanm/projects/abp/adblockplusui/browserify-css/js'
https://codereview.adblockplus.org/29710555/diff/29710556/package.json File package.json (right): https://codereview.adblockplus.org/29710555/diff/29710556/package.json#newcode11 package.json:11: "bundle:desktop-options": "eslint ./js/**/*.js && echo '/* eslint-disable */// BUNDLED FILE'>./desktop-options.js && browserify --node --no-bundle-external -t [ browserify-css --minify=true --output skin/desktop-options.css ] js/desktop-options.js >> ./desktop-options.js", On 2018/02/27 12:06:02, saroyanm wrote: > This throws and error for me: > Error: Cannot find module '../css/desktop-options.css' from > '/home/saroyanm/projects/abp/adblockplusui/browserify-css/js' did you run `npm install` before trying to bundle ?
https://codereview.adblockplus.org/29710555/diff/29710556/package.json File package.json (right): https://codereview.adblockplus.org/29710555/diff/29710556/package.json#newcode11 package.json:11: "bundle:desktop-options": "eslint ./js/**/*.js && echo '/* eslint-disable */// BUNDLED FILE'>./desktop-options.js && browserify --node --no-bundle-external -t [ browserify-css --minify=true --output skin/desktop-options.css ] js/desktop-options.js >> ./desktop-options.js", On 2018/02/27 12:31:05, a.giammarchi wrote: > On 2018/02/27 12:06:02, saroyanm wrote: > > This throws and error for me: > > Error: Cannot find module '../css/desktop-options.css' from > > '/home/saroyanm/projects/abp/adblockplusui/browserify-css/js' > > did you run `npm install` before trying to bundle ? Yes. "npm i" was the one throwing error.
https://codereview.adblockplus.org/29710555/diff/29710556/package.json File package.json (right): https://codereview.adblockplus.org/29710555/diff/29710556/package.json#newcode11 package.json:11: "bundle:desktop-options": "eslint ./js/**/*.js && echo '/* eslint-disable */// BUNDLED FILE'>./desktop-options.js && browserify --node --no-bundle-external -t [ browserify-css --minify=true --output skin/desktop-options.css ] js/desktop-options.js >> ./desktop-options.js", On 2018/02/27 12:32:58, saroyanm wrote: > On 2018/02/27 12:31:05, a.giammarchi wrote: > > On 2018/02/27 12:06:02, saroyanm wrote: > > > This throws and error for me: > > > Error: Cannot find module '../css/desktop-options.css' from > > > '/home/saroyanm/projects/abp/adblockplusui/browserify-css/js' > > > > did you run `npm install` before trying to bundle ? > > Yes. > "npm i" was the one throwing error. can I see your output after `npm run bundle` and also can you please double check you have the `css/desktop-options.css` file in the new `css` folder? I have no issues with this script, thanks. edit: actually, I wonder if it's about the squared brackets syntax ... I'll try on Linux and see if that works.
https://codereview.adblockplus.org/29710555/diff/29710556/package.json File package.json (right): https://codereview.adblockplus.org/29710555/diff/29710556/package.json#newcode11 package.json:11: "bundle:desktop-options": "eslint ./js/**/*.js && echo '/* eslint-disable */// BUNDLED FILE'>./desktop-options.js && browserify --node --no-bundle-external -t [ browserify-css --minify=true --output skin/desktop-options.css ] js/desktop-options.js >> ./desktop-options.js", On 2018/02/27 12:55:19, a.giammarchi wrote: > On 2018/02/27 12:32:58, saroyanm wrote: > > On 2018/02/27 12:31:05, a.giammarchi wrote: > > > On 2018/02/27 12:06:02, saroyanm wrote: > > > > This throws and error for me: > > > > Error: Cannot find module '../css/desktop-options.css' from > > > > '/home/saroyanm/projects/abp/adblockplusui/browserify-css/js' > > > > > > did you run `npm install` before trying to bundle ? > > > > Yes. > > "npm i" was the one throwing error. > > can I see your output after `npm run bundle` and also can you please double > check you have the `css/desktop-options.css` file in the new `css` folder? I > have no issues with this script, thanks. > > edit: actually, I wonder if it's about the squared brackets syntax ... I'll try > on Linux and see if that works. actually no, it works for me in Linux too. The only issue I have is that we have a CSS rule about needing a space or tab after the /* in the file but the esclamation mark is a common technique to preserve comments while processing files. We should really consider changing that rule.
https://codereview.adblockplus.org/29710555/diff/29710556/package.json File package.json (right): https://codereview.adblockplus.org/29710555/diff/29710556/package.json#newcode11 package.json:11: "bundle:desktop-options": "eslint ./js/**/*.js && echo '/* eslint-disable */// BUNDLED FILE'>./desktop-options.js && browserify --node --no-bundle-external -t [ browserify-css --minify=true --output skin/desktop-options.css ] js/desktop-options.js >> ./desktop-options.js", On 2018/02/27 13:20:59, a.giammarchi wrote: > On 2018/02/27 12:55:19, a.giammarchi wrote: > > On 2018/02/27 12:32:58, saroyanm wrote: > > > On 2018/02/27 12:31:05, a.giammarchi wrote: > > > > On 2018/02/27 12:06:02, saroyanm wrote: > > > > > This throws and error for me: > > > > > Error: Cannot find module '../css/desktop-options.css' from > > > > > '/home/saroyanm/projects/abp/adblockplusui/browserify-css/js' > > > > > > > > did you run `npm install` before trying to bundle ? > > > > > > Yes. > > > "npm i" was the one throwing error. > > > > can I see your output after `npm run bundle` and also can you please double > > check you have the `css/desktop-options.css` file in the new `css` folder? I > > have no issues with this script, thanks. > > > > edit: actually, I wonder if it's about the squared brackets syntax ... I'll > try > > on Linux and see if that works. > > actually no, it works for me in Linux too. The only issue I have is that we have > a CSS rule about needing a space or tab after the /* in the file but the > esclamation mark is a common technique to preserve comments while processing > files. > > We should really consider changing that rule. actually, never mind. Using the `/*!` in the css/desktop-options.css file doesn't cause any trouble (yet) so yes, this works for me in both macOS and Linux. Maybe @greiner can help here counter verifying ?
https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.... js/desktop-options.js:23: require("../css/desktop-options.css"); We already declare it in desktop-options.html so why do we also need to require it here? https://codereview.adblockplus.org/29710555/diff/29710556/package.json File package.json (right): https://codereview.adblockplus.org/29710555/diff/29710556/package.json#newcode11 package.json:11: "bundle:desktop-options": "eslint ./js/**/*.js && echo '/* eslint-disable */// BUNDLED FILE'>./desktop-options.js && browserify --node --no-bundle-external -t [ browserify-css --minify=true --output skin/desktop-options.css ] js/desktop-options.js >> ./desktop-options.js", What's the reason for minifying the code? I'm not aware of any benefits we'd get out of doing it. Is minification the only functionality that we need browserify-css for or is its bundling different from how CSS preprocessors do it? https://codereview.adblockplus.org/29710555/diff/29710556/package.json#newcode11 package.json:11: "bundle:desktop-options": "eslint ./js/**/*.js && echo '/* eslint-disable */// BUNDLED FILE'>./desktop-options.js && browserify --node --no-bundle-external -t [ browserify-css --minify=true --output skin/desktop-options.css ] js/desktop-options.js >> ./desktop-options.js", On 2018/02/27 13:25:55, a.giammarchi wrote: > Maybe @greiner can help here counter verifying ? Running `npm install` and `npm run bundle` works for me and doesn't throw any errors.
https://codereview.adblockplus.org/29710555/diff/29710556/package.json File package.json (right): https://codereview.adblockplus.org/29710555/diff/29710556/package.json#newcode11 package.json:11: "bundle:desktop-options": "eslint ./js/**/*.js && echo '/* eslint-disable */// BUNDLED FILE'>./desktop-options.js && browserify --node --no-bundle-external -t [ browserify-css --minify=true --output skin/desktop-options.css ] js/desktop-options.js >> ./desktop-options.js", On 2018/02/27 13:59:21, Thomas Greiner wrote: > On 2018/02/27 13:25:55, a.giammarchi wrote: > > Maybe @greiner can help here counter verifying ? > > Running `npm install` and `npm run bundle` works for me and doesn't throw any > errors. Sorry. It works for me as well, the issue was that importing the changes were not renaming the skin/desktop-options.css but rather updating skin/desktop-options.css
hopefully answered all questions, glad it works for both now. https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.... js/desktop-options.js:23: require("../css/desktop-options.css"); On 2018/02/27 13:59:21, Thomas Greiner wrote: > We already declare it in desktop-options.html so why do we also need to require > it here? look closer :-) what we ship is skin/desktop-options.css which is the result of the bundling. Here we're associating dedicated CSS to this JS file. This is a first step. The file css/desktop-options.css can be now split in sub modules and all be imported right in te CSS file when/if needed. All this JS file needs to do though, is just to specify which css entry point it needs. With a monolith, it's difficult to see advantages. Think about every sub component required in this file, it will bring in its own associated css and nothing else. As result, the `skin/desktop-options.css` file will be the bundle of all required modules and files, and each JS module can optionally bring in its own CSS rules. I have already modules with dedicated CSS. How else could I bring those in? I find using a single entry point to instrument bundles is convenient. Happy to listen to alternatives. https://codereview.adblockplus.org/29710555/diff/29710556/package.json File package.json (right): https://codereview.adblockplus.org/29710555/diff/29710556/package.json#newcode11 package.json:11: "bundle:desktop-options": "eslint ./js/**/*.js && echo '/* eslint-disable */// BUNDLED FILE'>./desktop-options.js && browserify --node --no-bundle-external -t [ browserify-css --minify=true --output skin/desktop-options.css ] js/desktop-options.js >> ./desktop-options.js", On 2018/02/27 13:59:21, Thomas Greiner wrote: > What's the reason for minifying the code? I'm not aware of any benefits we'd get > out of doing it. it makes instantly obvious to developers they should not change skin/*.css files, only css/*.css files. It's helpful to avoid losing work. If you see a minified file you instantly know that's not the source you want to edit. For JS is different, we want to eventually understand errors without needing to ship source maps. In CSS we don't need that because the debugger already shows associated styles. > > Is minification the only functionality that we need browserify-css for or is its > bundling different from how CSS preprocessors do it? browserify-css understand JS files that require CSS and create the bundle out of all dependencies. The CSS file can use also @import statements and these will be bundled too. Once desktop-options.js will be slip in different files, one pre section, as example, simply requiring it will bring in all sub modules and produce a single CSS file. browserify-css does exactly what browaserify does, but it's for CSS instead. The result is a single skin/desktop-options.css file created after bringing in everything needed. Again, it's not obvious with a monolith but I'll become instantly obvious how/why it's used once I land my first component.
https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.... js/desktop-options.js:23: require("../css/desktop-options.css"); The problem I see is that this approach requires us to manually maintain multiple references: 1. Reference to /skin/desktop-options.css in /desktop-options.html 2. Reference to /desktop-options.js in /desktop-options.html 3. Reference to /css/desktop-options.css in /js/desktop-options.js 4. Reference to /skin/desktop-options.css as output of browserify-css That means that a. we need to keep (1) and (2) in sync (both are required for component to work) b. we need to keep (1) and (4) in sync (both use same file name) That, in turn, means that making changes to either (1), (2) or (4) could lead to severe breakage. Therefore I'd recommend to make the above mentioned implicit dependencies explicit to avoid such breakage. https://codereview.adblockplus.org/29710555/diff/29710556/package.json File package.json (right): https://codereview.adblockplus.org/29710555/diff/29710556/package.json#newcode11 package.json:11: "bundle:desktop-options": "eslint ./js/**/*.js && echo '/* eslint-disable */// BUNDLED FILE'>./desktop-options.js && browserify --node --no-bundle-external -t [ browserify-css --minify=true --output skin/desktop-options.css ] js/desktop-options.js >> ./desktop-options.js", On 2018/02/27 14:19:36, a.giammarchi wrote: > it makes instantly obvious to developers they should not change skin/*.css > files, only css/*.css files. > > It's helpful to avoid losing work. If you see a minified file you instantly know > that's not the source you want to edit. I see your point but it sounds like we're misusing minification for a purpose it's not intended for. Aren't there ways to achieve this without making the output code unreadable?
https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.... js/desktop-options.js:23: require("../css/desktop-options.css"); On 2018/02/27 19:41:54, Thomas Greiner wrote: > The problem I see is that this approach requires us to manually maintain > multiple references: > 1. Reference to /skin/desktop-options.css in /desktop-options.html > 2. Reference to /desktop-options.js in /desktop-options.html > 3. Reference to /css/desktop-options.css in /js/desktop-options.js > 4. Reference to /skin/desktop-options.css as output of browserify-css > > That means that > a. we need to keep (1) and (2) in sync (both are required for component to work) > b. we need to keep (1) and (4) in sync (both use same file name) > > That, in turn, means that making changes to either (1), (2) or (4) could lead to > severe breakage. Therefore I'd recommend to make the above mentioned implicit > dependencies explicit to avoid such breakage. 1 and 2 are already there, those are the bundled version of the desktop-options page as a whole. I'd like to remind you I'm trying to gradually bring in bundling, hence basing solutions around what we have already and works. Those dependencies are also already explicit because the html file needs to point at one CSS and one JS, no matter which build system we chose, but I'm not sure I've got this point. https://codereview.adblockplus.org/29710555/diff/29710556/package.json File package.json (right): https://codereview.adblockplus.org/29710555/diff/29710556/package.json#newcode11 package.json:11: "bundle:desktop-options": "eslint ./js/**/*.js && echo '/* eslint-disable */// BUNDLED FILE'>./desktop-options.js && browserify --node --no-bundle-external -t [ browserify-css --minify=true --output skin/desktop-options.css ] js/desktop-options.js >> ./desktop-options.js", On 2018/02/27 19:41:54, Thomas Greiner wrote: > On 2018/02/27 14:19:36, a.giammarchi wrote: > > it makes instantly obvious to developers they should not change skin/*.css > > files, only css/*.css files. > > > > It's helpful to avoid losing work. If you see a minified file you instantly > know > > that's not the source you want to edit. > > I see your point but it sounds like we're misusing minification for a purpose > it's not intended for. Aren't there ways to achieve this without making the > output code unreadable? I don't see what's the point in reading shipped CSS but the way to not minify it is just to not use that --minify=true flag. I'm OK with it, but it'll make it less obvious which file is the CSS source.
https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.... js/desktop-options.js:23: require("../css/desktop-options.css"); On 2018/02/28 08:10:18, a.giammarchi wrote: > 1 and 2 are already there, those are the bundled version of the desktop-options page as a whole. That's true. I wrongly assumed it was part of your goal to reduce such implicit dependencies because why else would you refer to CSS files from a JavaScript file? Because if the goal is to merely bundle CSS files, we don't need this `require()` here, or am I missing something? We could instead write something like `sass css/desktop-options.css skin/desktop-options.css` to clarify that JavaScript and CSS files are independent of each other and so that both input and output are defined in a single spot. > Those dependencies are also already explicit because the html file needs to > point at one CSS and one JS, no matter which build system we chose, but I'm not > sure I've got this point. I mean the dependencies between (1) and (2) as well as between (1) and (4). Let me give you an example of what I mean. Currently renaming the CSS file requires the following manual steps: - Rename /skin/desktop-options.css - Change reference in desktop-options.html - Adapt mapping in other repositories when doing a dependency update With the suggested approach we're introducing one additional manual step: - Remove /skin/desktop-options.css - Change output of browserify-css - Change reference in desktop-options.html - Adapt mapping in other repositories when doing a dependency update But, as mentioned above, if the goal is not to reduce such implicit dependencies, we can ignore this for now. https://codereview.adblockplus.org/29710555/diff/29710556/package.json File package.json (right): https://codereview.adblockplus.org/29710555/diff/29710556/package.json#newcode11 package.json:11: "bundle:desktop-options": "eslint ./js/**/*.js && echo '/* eslint-disable */// BUNDLED FILE'>./desktop-options.js && browserify --node --no-bundle-external -t [ browserify-css --minify=true --output skin/desktop-options.css ] js/desktop-options.js >> ./desktop-options.js", On 2018/02/28 08:10:18, a.giammarchi wrote: > I don't see what's the point in reading shipped CSS but the way to not minify it > is just to not use that --minify=true flag. I'm OK with it, but it'll make it > less obvious which file is the CSS source. Again, I do think it's a good idea to make it obvious which files were auto-generated. I'm just saying that this is not the purpose of minification and if this turns out to be an actual problem, we can probably find more suitable ways to tackle it (e.g. detect and warn of manual modifications at build time; establishing conventions such as a dedicated output directory). Regarding what's the point of shipping readable CSS: Not minifying the code means that users can easily see what code we ship. I'm not saying that this is a requirement, it's more of a nice-to-have so we could ignore it if there's a benefit in doing so.
https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.... js/desktop-options.js:23: require("../css/desktop-options.css"); On 2018/02/28 12:23:14, Thomas Greiner wrote: > On 2018/02/28 08:10:18, a.giammarchi wrote: > > 1 and 2 are already there, those are the bundled version of the > desktop-options page as a whole. > > why else would you refer to CSS files from a JavaScript file? it's for components. Components have related CSS. You can define CSS when/if needed inside the JS file in various ways, or define the relationship outside JS in other ways. Since there is usually always some CSS associated to some JS I didn't find the approach so fancy, it actually makes more sense than many other solutions I've seen in this or that library. > Because if the goal is to merely bundle CSS files, we don't need this > `require()` here, or am I missing something? We could instead write something > like `sass css/desktop-options.css skin/desktop-options.css` to clarify that > JavaScript and CSS files are independent of each other and so that both input > and output are defined in a single spot. That's the thing. Components are not really independent from the CSS they need. A CSS alone doesn't bother much, a JS targeting UI without its related CSS is an issue. If you have better alternatives please go ahead, I was just trying to keep it simple: 1 js => 1 css ... the rest is brought in by browserify, that's it. > Let me give you an example of what I mean. Currently renaming the CSS file > requires the following manual steps: > - Rename /skin/desktop-options.css > - Change reference in desktop-options.html > - Adapt mapping in other repositories when doing a dependency update > > With the suggested approach we're introducing one additional manual step: > - Remove /skin/desktop-options.css no need to remove, it's a build target. If you are talking about hgignore then it's a one-off per section/page/component easy to spot in reviews. > - Change output of browserify-css no need to change that, the package.json script is a copy/paste one. True that you need to change the name though, but it's one name that via ctrl+d in editors is snappy to do ;-) > - Change reference in desktop-options.html no need to do that. Indeed, there are no changes in desktop-options.html because it will only bring in 1 js and 1 css as it does already. > - Adapt mapping in other repositories when doing a dependency update No need to adapt much, the bundling produces exact same result we have now: 1 js and 1 css. > But, as mentioned above, if the goal is not to reduce such implicit > dependencies, we can ignore this for now. I am happy to bring in something else but everything I've investigated requires bigger manual configurations in a way or another, there is less control per js file, and optionally its associated css, and less control over the generated output which is still pretty clean with browserify (comapred to what WebPack produces, as example). But again, if there are better alternatives, let's do it 'cause I need modular css for new components nad it's not possible right now. https://codereview.adblockplus.org/29710555/diff/29710556/package.json File package.json (right): https://codereview.adblockplus.org/29710555/diff/29710556/package.json#newcode11 package.json:11: "bundle:desktop-options": "eslint ./js/**/*.js && echo '/* eslint-disable */// BUNDLED FILE'>./desktop-options.js && browserify --node --no-bundle-external -t [ browserify-css --minify=true --output skin/desktop-options.css ] js/desktop-options.js >> ./desktop-options.js", On 2018/02/28 12:23:14, Thomas Greiner wrote: > On 2018/02/28 08:10:18, a.giammarchi wrote: I don't have strong feelings here in ABP case but if I can comment the following: > users can easily see what code we ship. they can do it already because everything is open source here. AFAIK everyone on the web minifies their projects in production so I don't see it as an issue. Users don't look source code, if they do, they have a public repository to see everything. We're introducing bundling which means we'll inevitably have not easy to read bundled code (but easier to read modules source code).
I think the crux of this review is that it's not only introducing CSS modules/bundles but also additional parts such as minification as well an attempt to introduce dependencies between CSS and JavaScript. So we can finish this review in various ways: a. Only include code necessary for bundling CSS files b. Agree on approaches for distinguishing bundles from source code as well as for establishing dependencies between CSS and JavaScript files c. Land as-is and agree on those approaches afterwards I'll leave it up to you which way you prefer since we want to land this sooner. And for that (b) seems unrealistic. https://codereview.adblockplus.org/29710555/diff/29710556/.hgignore File .hgignore (right): https://codereview.adblockplus.org/29710555/diff/29710556/.hgignore#newcode3 .hgignore:3: skin/desktop-options.js There is no skin/desktop-options.js so you probably meant skin/desktop-options.css https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.... js/desktop-options.js:23: require("../css/desktop-options.css"); On 2018/02/28 13:01:03, a.giammarchi wrote: > That's the thing. Components are not really independent from the CSS they need. > > A CSS alone doesn't bother much, a JS targeting UI without its related CSS is an > issue. Of course they're not independent but this `require()` merely causes the CSS file to be generated, it doesn't actually cause the CSS file to be included on the page. > If you have better alternatives please go ahead, I was just trying to keep it > simple: 1 js => 1 css ... the rest is brought in by browserify, that's it. All you're doing here is bundle the CSS which can also be achieved using a CSS precompiler, as I mentioned. If we want to establish a dependency between CSS and JavaScript, which we agree we want to do, it's only half the work to ensure that the CSS file is generated because we also need to make sure that the CSS file is included in the page. > I am happy to bring in something else but everything I've investigated requires > bigger manual configurations in a way or another, there is less control per js > file, and optionally its associated css, and less control over the generated > output which is still pretty clean with browserify (comapred to what WebPack > produces, as example). > > But again, if there are better alternatives, let's do it 'cause I need modular > css for new components nad it's not possible right now. We already agree on the bundling anyway so if we focus on that now and tackle the dependencies discussion later, we could already get this landed to get modules. https://codereview.adblockplus.org/29710555/diff/29710556/package.json File package.json (right): https://codereview.adblockplus.org/29710555/diff/29710556/package.json#newcode11 package.json:11: "bundle:desktop-options": "eslint ./js/**/*.js && echo '/* eslint-disable */// BUNDLED FILE'>./desktop-options.js && browserify --node --no-bundle-external -t [ browserify-css --minify=true --output skin/desktop-options.css ] js/desktop-options.js >> ./desktop-options.js", On 2018/02/28 13:01:03, a.giammarchi wrote: > AFAIK everyone on the web minifies their projects in production so I don't see > it as an issue. Everyone using it is not an argument. They may be using it to reduce the amount of network traffic or make it download quicker, none of which is relevant for extensions. What about discussing this separately so that we can move this review forward? Because whatever we agree on would not only apply to our bundled CSS code but also our JavaScript code.
On 2018/03/07 13:10:22, Thomas Greiner wrote: > I think the crux of this review is that ... is that I've should've put the follow up review but I didn't. https://codereview.adblockplus.org/29712640/ This can be closed if we like/agree on the other one, which is simpler and keep things where these belongs. Apologies for the misunderstanding. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
