Start using JavaScript modularization tool in adblockplusui

[saroyanm]

Background

In order to organize JavaScript files better and make code modularized we need to agree on a tool and start making use of it.

What to change

Add JavaScript modularization tool to adblockplusui

[saroyanm]

Flattr uses Browserify, Platform is using WebPack

We want to consider ESM loader and/or Rollup as well.

[agiammarchi]

Thomas, Manvel, I know this is not ready yet but I really need this to be ready and resolved ASAP.

I have uploaded a proof of concept that is not fancy at all but already works.
​https://codereview.adblockplus.org/29705690

Major points there:

• zero config, we gradually enrich package.json scripts as we go
• graceful migration, it allows us to keep everything we have already exactly where it is. No third parts changes. No cross-team effort. We just create and ship bundles per each page/section, instead of monoliths
• no global scope pollution, and no runtime evaluation. Everything is bundled as IIFE [1] and with use strict directive preserved.

If you are happy with that code review diff please mark this as ready and/or eventually discuss what we could do better (a lot, I know, but I also think we should incrementally find the best solution).

Thanks for considerations.

[1] ​https://developer.mozilla.org/en-US/docs/Glossary/IIFE

[abpbot]

A commit referencing this issue has landed:
​Issue 6310 - Start using JavaScript modularization tool

[kzar]

I am not happy about this and the related changes:

• Why don't we simply use the WebPack module system that we already have for adblockpluschrome? We can import modules from adblockpluscore and other repositories there and bundle them all up at build time, why can't we do that for adblockplusui code too? (Hint we can and already do that for the message responder.)
• What about source maps? One large desktop-options.js file in the extension with no source maps sucks, we have source maps for the files bundled at build type by the WebPack system and this saves us a lot of time with debugging problems.
• We normally put this kind of tooling in the buildtools repository, why didn't we in this case?
• This issue has no integration notes or information, crazy for such a large change.
• I don't like that the adblockplusui bundle(s) won't be generated unless the ensure_dependencies.py script happens to update / clone the adblockplusui repository. During development that's quite often not the case, so this is going to mean an extra thing to remember during development.

Finally to mention it one more time, why don't we just combine adblockpluschrome with adblockplusui since the former is the only consumer of the later.

[agiammarchi]

Replying to kzar:

I am not happy about this and the related changes:

Since I've implemented most of the tooling so far, I'd like to write down our initial constrains:

• we didn't want to change anything around the release process. Unfortunately, we had some hiccups the first time, otherwise our intent was to have a release that should've worked as it always did before. Apologies for some inconvenience, I'm personally still happy at the end it eventually worked, since that was the plan all along: graceful migration.
• we cannot hook into parent module Webpack configuration without passing through the Chrome repository per each change we need. We wanted to be fast, but not obtrusive, so we implemented the minimal amount of necessary tooling, waiting to discuss further integration with Chrome repository later on.
• we are just at the beginning of UI modularization. While current monolith might be convenient, like static files are, we didn't want to be trapped behind such monolith structure with no ability to add new modules/components. You use Webpack indeed to simplify your life, we thought it was about the time for UI to start using tools too.

As side note, and here I'm speaking for myself, last time I've collaborated in the Chrome repository, I've been told ​it would've take some time to grant me rights to push to your repository.

That surely spurred me to find any solution that wouldn't have involved bearing with, or bothering, Chrome team each time, with the goal of speeding up our own migration plan.

Accordingly, these are my personal answers per each bullet point of yours:

• it makes little sense to use webpack in a submodule. We should be able to configure Webpack in our parent folder, but that will require much more effort, being used by various teams. We wanted to be free, at least at the beginning, and fast.
• we have shipped a monolith so far, and we keep shipping a monolith. We can surely improve as soon as we are coordinated, and source map are a great way to go. However, our current solution does not introduce a regression, we still provide a monolith that was OK until now.
• buildtools repository is probably out of UI scope and it's written in Python. We were also using already package.json and npm to handle our repository and its dependencies, which is why we kept it that way. If package.json stays, and Chrome repository also uses Webpack, which is a NodeJS based tool, I think we can follow same setup, as mentioned in your first point.
• most informations are in the README.md, as the code review can show. I can find information on our parent repository README.md too, which is the file they pointed me when I've asked about building the extension. I thought that would've been enough, but apparently it's not: how can we improve that?
• whenever the ensure_dependencies.py updates or clones the repository, the implicit npm install happens, and the postinstall script is invoked. UI bundled files will be there. Moreover, for development purpose, we have all watchers in place so that npm run watch is basically all we need to develop. This is all in the README.md (if not, please file a bug!).

I'd like to also underline we're following what's been discussed in yesterday Team Meeting: we're trying to move forward, as fast as we can, adding value for our team and our users, without aiming at perfection.

We chose the easy way to go, and for us it worked. There is surely room for improvements, and we're more than happy to discuss the details (but maybe not in this specific ticket?).

As last personal thoughts, since this came up while trying to release for tests, I have the feeling the elephant in the room in this case has been our release process, and not the way ABP UI bundles, in a non obtrusive way, its own components.

I hence hope we can tackle this issue and cross team ASAP, like I've mentioned in 6476, underlying what I believe could be an easy win (we branch releases, not the next master).

Thanks for your help in moving forward despite the dependencies hiccup.

[saroyanm]

I did noticed that Andrea commented already, but I did put a lot of effort already into this notes :)

Replying to kzar:

I am not happy about this and the related changes:

I understand that this is frustrating and we should have communicated this with adblockpluschrome better.

I think we should understand till which point can we be autonomous and both be happy to use tool we like, the idea was to provide same output as was before while using the modularization tool ABPUI team agree on.

• Why don't we simply use the WebPack module system that we already have for adblockpluschrome? We can import modules from adblockpluscore and other repositories there and bundle them all up at build time, why can't we do that for adblockplusui code too? (Hint we can and already do that for the message responder.)

I personally have nothing agains webpack and I'm fine if we will use it instead of browserify.

• What about source maps? One large desktop-options.js file in the extension with no source maps sucks, we have source maps for the files bundled at build type by the WebPack system and this saves us a lot of time with debugging problems.

I thought it make sense to use source map only in the development builds, I don't mind adding it for testing, but not sure if we need to generate source map for the final build.

• This issue has no integration notes or information, crazy for such a large change.

Agree, that needs to be updated.

• I don't like that the adblockplusui bundle(s) won't be generated unless the ensure_dependencies.py script happens to update / clone the adblockplusui repository. During development that's quite often not the case, so this is going to mean an extra thing to remember during development.

Finally to mention it one more time, why don't we just combine adblockpluschrome with adblockplusui since the former is the only consumer of the later.

Ideally we would be included as a submodule and have a build directory IMO which will then be included into the Adblockpluschrome builds in my understanding ?

In general I'd be interested in hearing @greiner's opinion regarding merging as soon he is back from the vacation, as he is more experienced than I'm in the integration of ABPUI and Adblockpluschrome repositories.

I'm happy if we come up with the solution that will work best for both of the modules, while still we will be able to maintain ABPUI without bothering you too much and improve the process of providing releases. Also as soon we will be able to accomplish that we will be able to provide high quality UI more faster.

Also we either need to introduce release branch on UI side or start using "next" branch in order to keep the development flow and not interrupt development, this changes ended up in the release I think merely because the output, that is same. But fare enough that change should have went to other branch instead, which we will figure out for the next release, how we will keep the development flow during the release.

[sebastian]

Replying to agiammarchi:

As side note, and here I'm speaking for myself, last time I've collaborated in the Chrome repository, I've been told ​it would've take some time to grant me rights to push to your repository.

That surely spurred me to find any solution that wouldn't have involved bearing with, or bothering, Chrome team each time, with the goal of speeding up our own migration plan.

Everybody is welcome to contribute to adblockpluschrome. The lack of push access shouldn't make any difference regarding the appreciation of contributions, or in bothering those who have push access. Whether you have push access or not, the module owner (and/or peer) has to review your changes. Once your patch has been reviewed, it doesn't make much of a difference who is pushing the patch.

I'd like to also underline we're following what's been discussed in yesterday Team Meeting: we're trying to move forward, as fast as we can, adding value for our team and our users, without aiming at perfection.

I don't see how a solution that creates controversy and will potentially backfire later, is contributing to a more efficient process. Of course we could just put a blind eye to it, deal with the consequences later and while on it, abandon code review altogether, and resort to a sledge hammer when it comes to integrating the UI. The intermediate result might appear desired to anybody not understanding the development process, but it would be quite unproductive (and puts the quality of our product at risk) in the long run.

We chose the easy way to go, and for us it worked.

As much as autonomy is desired, adblockplusui is not a greenfield project. It has to integrate smoothly (on different levels) with adblockpluschrome. Merging the repositories seems to make this easier to enforce, while removing some overhead. But as long as we develop the UI in a separate repository, we have to make sure on a different level that our processes and infrastructure are aligned.

---

Regarding the change itself, I agree with kzar that the chosen approach doesn't quite fit in with our build process. It seems compromises were made in favor to keep the integration on the adblockpluschrome side minimal. However, as long as they make sense and are documented, changes on our end in order to properly integrate a dependency are no concern, and in fact would have been much preferable over that Rube Goldberg machine approach that we ended up with.

Ideally, we'd like to see the adblockpluschrome and adblockplusui repositories merge, eliminating the need for integrating the UI as an external component, configuring UI modules along other modules we have in adblockpluschrome through our build configuration which uses webpack under the hood.

However, even with adblockplusui being developed in a separate repository, we could still configure modules through our module mechanism in adblockpluschrome, the same way adblockpluscore is integrated. This also has the advantage that all modules to be used in the same context end up in the same bundle regardless where they come from, and thanks to webpack we'd get source maps for free.

Finally, if we really want to package the UI as an external component, the proper way seems to be generating an npm module with webpack. But I'm uncertain if this direction is the way to go. However, it seems at least less a hack than what has been implemented now.

[agiammarchi]

Replying to sebastian:

Replying to agiammarchi:
Once your patch has been reviewed, it doesn't make much of a difference who is pushing the patch.

I totally agree, which is why I don't understand the necessity of the extra step about sending the patch to push.

We're all busy, with different working time.

I don't see how a solution that creates controversy and will potentially backfire later,

A bit of a speculation. Like I've said, an Manvel too, the idea was to be fully transparent and at the end we managed to do that.

No backfiring, you consume exactly what you were consuming before. No risk, just a dependency hiccup due 3rd parts bundlers we quickly fixed.

The change was about using dependencies instead of devDependncies in a single package.json file. We missed that initially, but fixed it.

I'd also appreciate a bit less passive aggressive conversations, because we are all aiming for the best results and quality here, so the following sentence was really unnecessary:

Of course we could just put a blind eye to it, deal with the consequences later and while on it, abandon code review altogether, and resort to a sledge hammer when it comes to integrating the UI. The intermediate result might appear desired to anybody not understanding the development process, but it would be quite unproductive (and puts the quality of our product at risk) in the long run.

Thanks for keeping the discussion more realistic, less speculative, and respectful.

As much as autonomy is desired, adblockplusui is not a greenfield project. It has to integrate smoothly (on different levels) with adblockpluschrome.

Everything we did was intended to integrate smoothly.

We produce and write software, one issue we didn't think of happened, and it was fixed right away without real consequences.

Merging the repositories seems to make this easier to enforce, while removing some overhead.

While I agree merging might be the best scenario for the project, if doing so it means we double the amount of developers needed per each review, I think we'll be at least twice as slow, and I'm not sure that's desirable. Hopefully that won't be the case.

But as long as we develop the UI in a separate repository, we have to make sure on a different level that our processes and infrastructure are aligned.

Again, that was the intent, to keep the process as it was. We definitively should've communicated better how we were planning to do that.

It seems compromises were made in favor to keep the integration on the adblockpluschrome side minimal. However, as long as they make sense and are documented, changes on our end in order to properly integrate a dependency are no concern, and in fact would have been much preferable over that Rube Goldberg machine approach that we ended up with.

I like the metaphor, but browserify has been an industry standard and it was relatively straight forward to bring it in. Nothing complex at all, the issue was the 3rd parts python script skipping development and ignoring bundle scripts. We realized it too late.

I've also privately talked to @kzar days ago and agreed about merging and using Webpack would be most likely ideal.

It's not by accident that the whole structure, agreed and discussed already in UI, is also ready, out of the box, to be integrated with Webpack. We are creating specific folders to target.

We also kept the Webpack integration idea in mind.

However, even with adblockplusui being developed in a separate repository, we could still configure modules through our module mechanism in adblockpluschrome, the same way adblockpluscore is integrated. This also has the advantage that all modules to be used in the same context end up in the same bundle regardless where they come from, and thanks to webpack we'd get source maps for free.

If you could load ./js/desktop-options.js as CJS module then we're already compatible with this proposal.

We didn't know how much of an effort or how many changes would've been needed, so we did things the way we did to move faster and be transparent for the usual process.

Finally, if we really want to package the UI as an external component, the proper way seems to be generating an npm module with webpack.

We are creating Custom Elements, but there's no standard way to publish/bundle them with CSS. If we could agree on a way convenient for everyone, that'd be awesome.

But I'm uncertain if this direction is the way to go. However, it seems at least less a hack than what has been implemented now.

What we implemented was a way to move forward quickly without bothering other teams and now that we've fixed the dependency bit, I can say it worked very well too.

[kzar]

Replying to agiammarchi:

As side note, and here I'm speaking for myself, last time I've collaborated in the Chrome repository, I've been told ​it would've take some time to grant me rights to push to your repository.

That surely spurred me to find any solution that wouldn't have involved bearing with, or bothering, Chrome team each time, with the goal of speeding up our own migration plan.

I took quite some time to reassure and encourage you both in the Hub issue and in IRC, to explain that I was happy to push patches for you until you had access and that it was nice you were taking the time to contribute.

I feel like you're implying that we rejected you or your work which is an unfair characterisation.

Furthermore saying things like "bearing with, or bothering, Chrome team each time" is pretty inflammatory, especially when you consider I really tried to help you. We're all on the same side, working towards the same goal of making Adblock Plus as good as we can.

[agiammarchi]

Replying to kzar:

I feel like you're implying that we rejected you or your work which is an unfair characterisation.

It sadden me that's the impression I gave you, and I've thanked you, and thank again, for your help.

Truth is, I've simply tried to simplify things for both your team and UI, and there was really nothing personal in the decision process.

But it seems nobody is considering everything we've done was in good faith and for the best interests of everyone.

Even me saying that I didn't want to bother per each change chrome team, specially during a phase where we've just started migrating, discussing, and changing many things and very quickly, is read as inflammatory, instead of a strategy we, as a team, took, to move faster.

Once again: we, as a team, agreed on this pattern with the goal of producing best compromise for everyone, keeping UI independent at this time, willing to integrate in the very near future with concrete proposals, while brainstorming solutions.

Asking you or anyone else to push patches would've meant you being aware of what these patches were about, the why, and the how, slowing us down.

Your team does not participate to UI meetings, and is not present in UI channel. Involving your branch for things we agreed as UI team would've required double amount of time to explain all details per each change, wouldn't you agree?

Again. we are in a migration phase where we want to be free to also experiment and change things as fast as possible and as we go.

I believe your branch is not the best place to do this, at least not at this time, and doing the way we did, beside one single minor issue instantly fixed, stole zero extra time to your team, which was our goal at this stage.

It's clear by now things done this way are apparently not ideal for everyone and that's OK: let's discuss how to improve, keeping in mind what UI needs as a team, which is a bit different from what extension or core does (e.g. SASS, Custom Elements polyfills, etc).

We're all on the same side, working towards the same goal of making Adblock Plus as good as we can.

Absolutely, so, let's keep that in mind and move forward with concrete proposals that work best for everyone.

Is this closed ticket the best place to do that? Looks to me we're not really making progresses in here so, should we have a proper ticket to discuss constrains, needs, and propose solutions?

I hope so.

So let's do that as joint effort to move forward ... and as a team!

[sebastian]

Replying to agiammarchi:

I totally agree, which is why I don't understand the necessity of the extra step about sending the patch to push.

We're all busy, with different working time.

I don't see how emailing a patch is more effort for you than pushing the change to hg yourself. FWIW, I personally wouldn't even ask you to email the patch, but just download it from Rietveld. If the overhead on the module owner side becomes unmanageable, they can just decide to give you push access, but I don't see how that bothers you. Also note that our ​policy states to grant push access to regular contributors, not to anybody who is landing their first change.

While I agree merging might be the best scenario for the project, if doing so it means we double the amount of developers needed per each review, I think we'll be at least twice as slow, and I'm not sure that's desirable. Hopefully that won't be the case.

Nobody is talking about merging the UI and Platform modules, but merely the repositories where the code is located. If we just move the contents of adblockplusui into a ui subfolder in adblockpluschrome, changes inside that subfolder would remain to be reviewed by only the UI module owner/peer, while changes outside of that folder would remain to be reviewed by only the Platform module owner/peer.

I like the metaphor, but browserify has been an industry standard and it was relatively straight forward to bring it in.

Well, as far as I can tell, browserify has been replaced by webpack in most parts of the industry. Moreover, we are already using webpack and adding yet another asset bundler to the mix, adds unnecessarily complexity to our stack. The combination of the build steps and how they are chained is what makes it a Rube Goldberg machine.

We are creating Custom Elements, but there's no standard way to publish/bundle them with CSS. If we could agree on a way convenient for everyone, that'd be awesome.

With webpack you can include non-code files from an npm package, e.g. ~pkgname/css/styles.css.

Once again: we, as a team, agreed on this pattern with the goal of producing best compromise for everyone

Well, given the ongoing controversial discussion here, I don't think that we are close to a good compromise. And, again, I also don't think a solution that was implemented rapidly, but then causes us to spend as more time in such debates, makes us move forward faster.

[kzar]

Replying to agiammarchi:

Replying to kzar:

I feel like you're implying that we rejected you or your work which is an unfair characterisation.

It sadden me that's the impression I gave you...

I'm glad that isn't the case, thanks for clarifying.

[agiammarchi]

Replying to sebastian:

I don't see how emailing a patch is more effort for you than pushing the change to hg yourself.

It's simple. You, rightly, don't blindly apply patches. At least, that's not what I would do in my repository.

Nobody is talking about merging the UI and Platform modules, but merely the repositories where the code is located. If we just move the contents of adblockplusui into a ui subfolder in adblockpluschrome, changes inside that subfolder would remain to be reviewed by only the UI module owner/peer, while changes outside of that folder would remain to be reviewed by only the Platform module owner/peer.

Apologies but then I don't see any difference then to what we do already.

adding yet another asset bundler to the mix, adds unnecessarily complexity to our stack.

If by "our" stack you mean adblockpluschrome here, there was literally nothing you had to do, which was our goal.

Moreover, we used browserify only as a quick way to move forward with our needs at this stage.

You can indeed find every LGTM mentioning "for now, this is good enough/serves us pretty well".

We knew already it wouldn't have scaled in the long term but again, our idea has always been to easily drop browserify and integrate with Webpack on our parent folder in the very near future.

Regardless, to bring in browserify took 1/10 of the time I've spent in this thread and it took also nothing to ​update `devDepepndencies` to `dependencies` which is the only hiccup we had in this release.

With webpack you can include non-code files from an npm package, e.g. ~pkgname/css/styles.css.

Interesting. But do we need to change webpack per each new Custom Elements? Some of them might not even have a related CSS file, but at the end of the day all of them needs to be bundled as desktop-options.js/desktop-options.css.

If we can do that, then let's start already, there is a Custom Element (that I also need ASAP) ready for review. We could use that as base test to see how it works.

Well, given the ongoing controversial discussion here, I don't think that we are close to a good compromise. And, again, I also don't think a solution that was implemented rapidly, but then causes us to spend as more time in such debates, makes us move forward faster.

The fact we are having such conversation, which is anyway good because at some point we were willing to discuss this, has nothing to do with browserify or the way we bundled the same file you've always consumed: it's us deciding to talk about it, not a release blocker, nothing really risky, nothing is broken, no user ever affected, no drama at all (IMO).

At any point in time, this conversation would've happened regardless.

The release, meanwhile, happened so ... can we just move on to the next step with concrete proposals once Thomas is back?

It'd be great, thanks!

[sebastian]

Replying to agiammarchi:

Apologies but then I don't see any difference then to what we do already.

With both UI and platform code developed in the same repository, we can simplify the build steps and don't need to bother about integrating the UI as an external component. Moreover, it helps to streamline the release process (i.e. no dependency updates, synchronized code freeze, etc.).

[agiammarchi]

I still would like to discuss this with Thomas and Manvel though, and these are few personal outstanding issues I could quickly think of:

1. we won't be able to work, as separate project, on Gitlab, and land patches in Mercurial, because IIRC adblockpluschrome is not on Gitlab, neither planning to be there. I personally don't like much working with HG and I'm at least twice as fast, and way more confident, with git. That's just my own opinion/habit though, and if everyone else is happy with hg then I guess it's just a matter of time for me too.
2. will we have the rights to push and/or read private tickets/code reviews? Yesterday, as example, I was added in CC in another ticket, and after following all discussions and checked the code to understand what was going on, I've realized I could not even review (most important, learn) anything from code review. Feeling like a foreigner in the very same company you work for, and the very same repository you contribute to, is not the best feeling, IMO, so if merging would increase that feeling I know already that'd be frustrating.
3. we have different pages to target, and we use SASS to deal with CSS imports too (we used SASS to deal with variables too but since we've dropped Edge 15 we can use native CSS variables already). We need to bundle pages, not just JS, with optional assets. AFAIK every new file would require changes in the manifest too (thinking of source map here), which is also one of the reasons we went for the current strategy: no need to change anything in the manifest. As summary, if we'll ever end up creating just desktop-options.html/js/css, I'd say we wait for our component refactory to happen and only after we see what we've learned and what's the best solution to go. Unless, of course, Webpack can solve everything through its opinionated nature (if that's even the case, I know Webpack is actually very configurable but not so easy to configure).

Thanks in advance for any sort of follow up. I know release made everyone very busy, but since I am doing some refactory to be able to improve the UI, the sooner I know how to bundle my components, the better.

Regards.

[saroyanm]

Replying to agiammarchi:

I still would like to discuss this with Thomas and Manvel though, and these are few personal outstanding issues I could quickly think of:

Right, I think best would be to discuss this all together with ABPUI team, I personally not see yet issues of using browserify, but I think we can improve integration, whether we will use browserify, webpack or other tool it shouldn't matter much as soon we ensure that it's easily integratable, testable with the solution provided by platform team, currently I can't clearly judge the implications of the merging modules, so as I mentioned above I would be interested in the Thomas opinion here, we will probably move this discussion else where, anyway we will have a good solution, it's just a matter of time, it took some time to start handling translations on ABPUI side and I'm sure having right tools to help ABPUI move forward fast is beneficiary for everyone.

2. will we have the rights to push and/or read private tickets/code reviews? Yesterday, as example, I was added in CC in another ticket, and after following all discussions and checked the code to understand what was going on, I've realized I could not even review (most important, learn) anything from code review. Feeling like a foreigner in the very same company you work for, and the very same repository you contribute to, is not the best feeling, IMO, so if merging would increase that feeling I know already that'd be frustrating.

I'm very sure that this wasn't intended the way you felt and sorry for not making that clear enough, anyone in the company have right to view private reviews and tickets when interested The case you have mentioned was caused by the ticket description change, release rush. I'm sorry that it felt that way, I'm sure other people involved can confirm that as well, that it 100% were not intended that way you interpreted here and again sorry for not making that clear enough. In general having 1 person from the module to review relevant changes is enough, that why only me was added as CC to the review, probably because I was actively discussing the issue and the changes mostly were relevant to the parts I was working on lately and pushing.

We will come back to this issue soon when Thomas is back and I'm sure we will able to find compromises that work best for everyone, at least I'm sure having no modularization can be potentially be more error prone.

We have here several issues, we would like first discuss with team:

1. Improve branch management and find ways to use tools that UI team is happy with,
2. Introduce modularization that plays well with both platform tools and UI
3. Discuss possibility of merging the modules

Also having this ending up in the release branch is my fault, but not Andreas, as I was managing the relaese and gave a green light considering that the output is same and we shouldn't care much about it, but apparently it wasn't true, anyway we will fix all of that soon I'm sure.

[agiammarchi]

Replying to saroyanm:

I'm very sure that this wasn't intended the way you felt

Thanks Manvel. I am sure nobody did anything intentionally, I was rather sharing a situation that never happened before (at least since I'm working in ABPUI) and that beside wasting some time upfront made me incapable of learning about very core related stuff.

I've been told some particular discussion might happen in our hub, but I wasn't expecting eyeo developers unable to read code reviews, so that I've suddenly found myself 100% useless in there.

If there's a default list of authorized developers per each code review it'd be awesome to be part of such list.

If it's a thing of the code review tool itself, and one needs to manually add people, then it's OK, yet unexpected 'cause it was the first time and also 'cause AFAIK we have our code fully public anyway.

If that happens every time it's platform instead of UI though, and we get merged, my feelings/doubts for the second point remains.

[kzar]

Replying to agiammarchi:

... because IIRC adblockpluschrome is not on Gitlab, neither planning to be there.

​https://gitlab.com/eyeo/adblockplus/adblockpluschrome , ​http://hub.eyeo.com/issues/7352

I was added in CC in another ticket, and after following all discussions and
checked the code to understand what was going on, I've realized I could not
even review (most important, learn) anything from code review.

The issue (#6518) was marked Confidential, so it's our policy to mark corresponding codereviews as Private. With Rietveld (despite the UI being somewhat misleading) Private issues are only viewable by the author, reviewers and users who have been Cc'd. I did not add you as a reviewer or to Cc for the codereview since I did not need you to review the change, same as how I didn't add many other people at eyeo. Manvel is right that this was not meant as some kind of personal snub against you or any of the other people that I did not Cc.

[sebastian]

Replying to agiammarchi:

1. we won't be able to work, as separate project, on Gitlab, and land patches in Mercurial, because IIRC adblockpluschrome is not on Gitlab, neither planning to be there. I personally don't like much working with HG and I'm at least twice as fast, and way more confident, with git. That's just my own opinion/habit though, and if everyone else is happy with hg then I guess it's just a matter of time for me too.

FWIW, I feel that Mercurial provides a higher level of integrity and as a result makes common mistakes I've seen in teams working with Git less likely to happen. Also some commands (e.g. hg rebase -s <source> -d <destination> vs git rebase --onto <newbase> <oldparent> <head>) seem to be more convenient to use. However, I can relate that when you are used to Git, and then start working on a project using Mercurial, you feel a little frustrated having to start over again, learning and adaptng to a new VCS and figuring out how to accomplish tasks that you were used to know to do with Git. I have been there before, and it took me some time until I got comfortable with Mercurial and begun to appreciate it.

Anyway, if everybody else prefers Git, and in particular if this would hold us back here otherwise, I might be fine with abandoning Mercurial. However, FWIW, this is somewhat unrelated of merging the repositories. Even if adblockplusui keeps being developed as a separate repository, you cannot drop Mercurial as long as adblockpluschrome uses it. Otherwise, this will break our dependency mechanism.

However, I care about having a sufficient code review tool. While Rietveld is no longer maintained, and we want to migrate away from it for a while, draft comments and inter-patch diffs (i.e. seeing what changed in between different versions of a patch, along any inline comments) were the pain points when we were looking for alternatives. It seems the review tool provided by GitLab is insufficient. Rietveld (for the time being) or Gerrit would be better options. However, these tools work with Git as well.

2. will we have the rights to push and/or read private tickets/code reviews? Yesterday, as example, I was added in CC in another ticket, and after following all discussions and checked the code to understand what was going on, I've realized I could not even review (most important, learn) anything from code review. Feeling like a foreigner in the very same company you work for, and the very same repository you contribute to, is not the best feeling, IMO, so if merging would increase that feeling I know already that'd be frustrating.

Every staff member can access all private issues on Trac. However, Rietveld only allows owner/reviewers/CC of a private review to access it. Regardless, whether we merge the repositories or not, you might occasionally come across reviews you cannot access for that reason. The only way to avoid that would be adding every single developer to CC in every private review, which would cause quite some noise. However, if you want to look at a private review you can just ask to be added to CC.

As for pushing code, everybody who has push access to adblockplusui at the moment, will of course get push access to adblockpluschrome, if we merge the repositories.

3. we have different pages to target, and we use SASS to deal with CSS imports too (we used SASS to deal with variables too but since we've dropped Edge 15 we can use native CSS variables already). We need to bundle pages, not just JS, with optional assets. AFAIK every new file would require changes in the manifest too (thinking of source map here), which is also one of the reasons we went for the current strategy: no need to change anything in the manifest. As summary, if we'll ever end up creating just desktop-options.html/js/css, I'd say we wait for our component refactory to happen and only after we see what we've learned and what's the best solution to go. Unless, of course, Webpack can solve everything through its opinionated nature (if that's even the case, I know Webpack is actually very configurable but not so easy to configure).

Any build steps required for the UI would be directly integrated with our build scripts, analogous to how non-UI code is bundled. For now, this means updating the metadata.* files, and if necessary adding functionality to buildtools (we have a dedicated developer to help with that).

[agiammarchi]

Replying to sebastian:

Thanks Sebastian. It looks like the basics to move forward and find appropriate compromises are already in.

We are discussing this on next Thursday with Thomas and Manvel and I am sure we'll follow up about this.

[Ross]

Just noting: This has been started and does not look to have had any adverse effects on the builds.

[greiner]

Are there any reasons for keeping this ticket open? By now, we have already started splitting up our code into modules using Browserify. We can always create new tickets for any of the other changes that were suggested in the comments section, if we need to.

[agiammarchi]

+1 on closing this

[saroyanm]

Replying to greiner:

Are there any reasons for keeping this ticket open? By now, we have already started splitting up our code into modules using Browserify. We can always create new tickets for any of the other changes that were suggested in the comments section, if we need to.

Seems like the ticket is closed.