Issue 6426 - Start using CSS modularization tool in adblockplusui

js/desktop-options.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 /* globals checkShareResource, getDocLink, i18nFormatDateTime, openSharePopup,
-           setLinks, E */
+           require, setLinks, E */
 
 "use strict";
 
+require("../css/desktop-options.css");

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

We already declare it in desktop-options.html so why do we also need to require
it here?

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

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.

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

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.

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

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.

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

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

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

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.
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.
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.
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 ;-)
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.
No need to adapt much, the bundling produces exact same result we have now: 1 js
and 1 css.
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.

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

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

+
 let subscriptionsMap = Object.create(null);
 let filtersMap = Object.create(null);
 let collections = Object.create(null);
 let acceptableAdsUrl = null;
 let acceptableAdsPrivacyUrl = null;
 let isCustomFiltersLoaded = false;
 let {getMessage} = browser.i18n;
 let {setElementText} = ext.i18n;
 let customFilters = [];
 let filterErrors = new Map([
@@ skipping 1432 matching lines @@
            "ui_warn_tracking"]
 });
 browser.runtime.sendMessage({
   type: "subscriptions.listen",
   filter: ["added", "disabled", "homepage", "lastDownload", "removed",
            "title", "downloadStatus", "downloading"]
 });
 
 window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
 window.addEventListener("hashchange", onHashChange, false);