Noissue - bundle in one operation to ensure no races

Noissue - bundle in one operation to ensure no races
Replaced by: https://codereview.adblockplus.org/29716587/

[a.giammarchi, 2018-03-07 11:24:46]

this should ensure watching never causes double files as result.

[Thomas Greiner, 2018-03-07 13:48:11]

Only added some notes on potential improvements for later.

LGTM but note that the commit message prefix should be "Noissue - " and that
noissue commits should include the review URL for context.

i.e.
Noissue - bundle in one operation to ensure no races
Review: https://codereview.adblockplus.org/29715740/

https://codereview.adblockplus.org/29715740/diff/29715741/README.md
File README.md (right):

https://codereview.adblockplus.org/29715740/diff/29715741/README.md#newcode113
README.md:113: ```
Suggestion: Maybe we shouldn't include any code here. Otherwise we always have
to keep it in sync with the actual code in package.json.

Do you think it's worth the duplication?

https://codereview.adblockplus.org/29715740/diff/29715741/package.json
File package.json (right):

https://codereview.adblockplus.org/29715740/diff/29715741/package.json#newcode11
package.json:11: "bundle:desktop-options": "eslint ./js/**/*.js && bash -c 'echo
\"/* eslint-disable */$(browserify --node --no-bundle-external
js/desktop-options.js)\">./desktop-options.js'",
Note: That's good for now but maybe we should start going beyond bash soon, now
that the build steps are becoming more complex and more difficult to read.

[a.giammarchi, 2018-03-07 14:09:01]

https://codereview.adblockplus.org/29715740/diff/29715741/README.md
File README.md (right):

https://codereview.adblockplus.org/29715740/diff/29715741/README.md#newcode113
README.md:113: ```
On 2018/03/07 13:48:11, Thomas Greiner wrote:
> Suggestion: Maybe we shouldn't include any code here. Otherwise we always have
> to keep it in sync with the actual code in package.json.
> 
> Do you think it's worth the duplication?

my idea is that the README explains what happens in specific package.json
scripts entries but there's no need whatsoever to duplicate this per each new
entry because they will all look the same.

So, if your concern is about needing to update the README if we change the
scripts, then maybe you are right, we don't need such duplication.

However, not everyone is super familiar with bash so that expanded with few
comments might help understanding it.

But then again, as explanation of how bash works, this is indeed not exhaustive
at all.

Maybe we can drop it as soon as we need to add new scripts and/or we change
something?

https://codereview.adblockplus.org/29715740/diff/29715741/package.json
File package.json (right):

https://codereview.adblockplus.org/29715740/diff/29715741/package.json#newcode11
package.json:11: "bundle:desktop-options": "eslint ./js/**/*.js && bash -c 'echo
\"/* eslint-disable */$(browserify --node --no-bundle-external
js/desktop-options.js)\">./desktop-options.js'",
On 2018/03/07 13:48:11, Thomas Greiner wrote:
> Note: That's good for now but maybe we should start going beyond bash soon,
now
> that the build steps are becoming more complex and more difficult to read.

one possible improvement is to have a single bash.js and/or bash.css entry to
use with that one liner and a `$SOURCE` and a `$DEST` variable so that a
bundle:something.js would be `npm run bash.js ./js/something.js ./something.js`
and it's easier/cleaner to add proper bundling instead of copy/pasting and
manually replacing each file.

At the end of the day, in a way or another, we need to update sources/targets
while migrating, then we can just switch to more complex bundlers once we've
fully migrated to the new structure.

Thoughts?

[Thomas Greiner, 2018-03-07 14:25:49]

https://codereview.adblockplus.org/29715740/diff/29715741/README.md
File README.md (right):

https://codereview.adblockplus.org/29715740/diff/29715741/README.md#newcode113
README.md:113: ```
On 2018/03/07 14:09:00, a.giammarchi wrote:
> my idea is that the README explains what happens in specific package.json
> scripts entries but there's no need whatsoever to duplicate this per each new
> entry because they will all look the same.
> 
> So, if your concern is about needing to update the README if we change the
> scripts, then maybe you are right, we don't need such duplication.
> 
> However, not everyone is super familiar with bash so that expanded with few
> comments might help understanding it.
> 
> But then again, as explanation of how bash works, this is indeed not
exhaustive
> at all.
> 
> Maybe we can drop it as soon as we need to add new scripts and/or we change
> something?

That can go hand-in-hand with my other comment because if we split off the
actual build logic into separate files we could document all of those
implementation details inline as we do with all of our other code.

We could then focus on the purpose and how to use each individual command here
in the README.

https://codereview.adblockplus.org/29715740/diff/29715741/package.json
File package.json (right):

https://codereview.adblockplus.org/29715740/diff/29715741/package.json#newcode11
package.json:11: "bundle:desktop-options": "eslint ./js/**/*.js && bash -c 'echo
\"/* eslint-disable */$(browserify --node --no-bundle-external
js/desktop-options.js)\">./desktop-options.js'",
On 2018/03/07 14:09:00, a.giammarchi wrote:
> On 2018/03/07 13:48:11, Thomas Greiner wrote:
> > Note: That's good for now but maybe we should start going beyond bash soon,
> now
> > that the build steps are becoming more complex and more difficult to read.
> 
> one possible improvement is to have a single bash.js and/or bash.css entry to
> use with that one liner and a `$SOURCE` and a `$DEST` variable so that a
> bundle:something.js would be `npm run bash.js ./js/something.js
./something.js`
> and it's easier/cleaner to add proper bundling instead of copy/pasting and
> manually replacing each file.
> 
> At the end of the day, in a way or another, we need to update sources/targets
> while migrating, then we can just switch to more complex bundlers once we've
> fully migrated to the new structure.
> 
> Thoughts?

Sounds like a workable approach and it's also pretty similar to how we use gulp
for building the Flattr extension (see
https://github.com/flattr/flattr-extension/blob/master/package.json).

I also agree with your assessment that we're not at the point yet where we
should avoid more complex build logic until we've fully migrated all of the
code. So for now forwarding npm commands to a simple script seems indeed ideal.

[a.giammarchi, 2018-03-07 14:48:34]

In order to avoid all sort of conflicts, since I've already pushed the SASS
related changes,
I've created a new review from latest master that simplifies the creation of new
bundles
and also explain in the README only what matters, not what Bash does :-)

https://codereview.adblockplus.org/29716587/

I am closing this as non applicable.