Issue 6426 - Start using CSS modularization tool in adblockplusui

package.json

Index: package.json
===================================================================
--- a/package.json
+++ b/package.json
@@ -3,21 +3,22 @@
   "private": true,
   "description": "Adblock Plus UI",
   "scripts": {
     "lint": "npm run lint:js",
     "lint:js": "eslint ./*.js ./lib/*.js ./ext/*.js",
     "prepare": "python2 ensure_dependencies.py",
     "start": "http-server & npm run watch",
     "bundle": "npm run bundle:desktop-options",
-    "bundle:desktop-options": "eslint ./js/**/*.js && echo '/* eslint-disable */// BUNDLED FILE'>./desktop-options.js && browserify --node --no-bundle-external js/desktop-options.js >> ./desktop-options.js",
+    "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",

[saroyanm, 2018/02/27 12:06:02]

This throws and error for me:
Error: Cannot find module '../css/desktop-options.css' from
'/home/saroyanm/projects/abp/adblockplusui/browserify-css/js'

[a.giammarchi, 2018/02/27 12:31:05]

did you run `npm install` before trying to bundle ?

[saroyanm, 2018/02/27 12:32:58]

Yes.
"npm i" was the one throwing error. 

[a.giammarchi, 2018/02/27 12:55:19]

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.

[a.giammarchi, 2018/02/27 13:20:59]

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.

[a.giammarchi, 2018/02/27 13:25:55]

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 ?

[Thomas Greiner, 2018/02/27 13:59:21]

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?

[Thomas Greiner, 2018/02/27 13:59:21]

Running `npm install` and `npm run bundle` works for me and doesn't throw any
errors.

[saroyanm, 2018/02/27 14:15:19]

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 

[a.giammarchi, 2018/02/27 14:19:36]

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.
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.

[Thomas Greiner, 2018/02/27 19:41:54]

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?

[a.giammarchi, 2018/02/28 08:10:18]

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.

[Thomas Greiner, 2018/02/28 12:23:14]

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.

[a.giammarchi, 2018/02/28 13:01:03]

I don't have strong feelings here in ABP case but if I can comment the
following:
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).

[Thomas Greiner, 2018/03/07 13:10:22]

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.

     "watch": "npm run watch:desktop-options",
-    "watch:desktop-options": "watch 'npm run bundle:desktop-options' ./js",
+    "watch:desktop-options": "watch 'npm run bundle:desktop-options' ./js ./css",
     "postinstall": "npm run bundle"
   },
   "devDependencies": {
     "browserify": "^16.1.0",
+    "browserify-css": "^0.14.0",
     "eslint": "^4.16.0",
     "eslint-config-eyeo": "^2.0.0",
     "http-server": "^0.11.1",
     "watch": "^1.0.2"
   }
 }