Issue 6310 - Start using JavaScript modularization tool

Issue 6310 - Start using JavaScript modularization tool

[Thomas Greiner, 2018-02-22 16:24:18]

I think this is a good approach to introduce modules with minimal amount of
side-effects. So if you want we can already get this one landed or do you intend
to apply this to more files first?

In any case, please make sure that desktop-options.js is removed though because
I'm seeing it marked with an "M". For that I'd suggest using `hg mv` to make it
explicit that the file has merely been moved.

[a.giammarchi, 2018-02-22 16:42:04]

On 2018/02/22 16:24:18, Thomas Greiner wrote:
> or do you intend to apply this to more files first?

nope, I'd like the process in place so I can start introducing JS modules and
components later on.

> In any case, please make sure that desktop-options.js is removed though
because
> I'm seeing it marked with an "M". For that I'd suggest using `hg mv` to make
it
> explicit that the file has merely been moved.

I have used `hg remove -f desktop-options.js` but that clearly didn't produce
the R in here.

I have the removal clear in my `hg status` though. I'll try to sort this out.

[a.giammarchi, 2018-02-22 17:23:42]

I have cleaned everything up.

[Thomas Greiner, 2018-02-22 18:29:23]

LGTM with an optional suggestion

https://codereview.adblockplus.org/29705690/diff/29705808/.hgignore
File .hgignore (right):

https://codereview.adblockplus.org/29705690/diff/29705808/.hgignore#newcode2
.hgignore:2: desktop-options.js
Suggestion: Note that this also matches js/desktop-options.js. While this
shouldn't cause any issues, it may still be unexpected.

Therefore I'd suggest changing it to:
^desktop-options.js

Alternatively, you could introduce a dedicated output directory so that we don't
have to keep track of each individual file in here. Otherwise we can do that
later on.

[saroyanm, 2018-02-22 18:54:32]

https://codereview.adblockplus.org/29705690/diff/29705808/package.json
File package.json (right):

https://codereview.adblockplus.org/29705690/diff/29705808/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 js/desktop-options.js >> ./desktop-options.js",
What does double ">>" mean in browserify, I thought the syntax is something like
"destination" > "target" ?

[saroyanm, 2018-02-22 19:07:28]

Suggestion and a detail.

https://codereview.adblockplus.org/29705690/diff/29705808/package.json
File package.json (right):

https://codereview.adblockplus.org/29705690/diff/29705808/package.json#newcode9
package.json:9: "start": "http-server",
Suggestion: What about watching changes while running local server ?
Any arguments against it ?
I think it's handy when you don't need to run watcher every time separately.

https://codereview.adblockplus.org/29705690/diff/29705808/package.json#newcode10
package.json:10: "bundle": "npm run bundle:desktop-options",
Detail: I think it will be helpful to document in README file how to create a
bundle and run watchers.

[a.giammarchi, 2018-02-23 08:51:38]

https://codereview.adblockplus.org/29705690/diff/29705808/.hgignore
File .hgignore (right):

https://codereview.adblockplus.org/29705690/diff/29705808/.hgignore#newcode2
.hgignore:2: desktop-options.js
On 2018/02/22 18:29:23, Thomas Greiner wrote:
> Suggestion: Note that this also matches js/desktop-options.js. While this
> shouldn't cause any issues, it may still be unexpected.
> 
> Therefore I'd suggest changing it to:
> ^desktop-options.js
> 
> Alternatively, you could introduce a dedicated output directory so that we
don't
> have to keep track of each individual file in here. Otherwise we can do that
> later on.

Happy to use the suggested change.

https://codereview.adblockplus.org/29705690/diff/29705808/package.json
File package.json (right):

https://codereview.adblockplus.org/29705690/diff/29705808/package.json#newcode9
package.json:9: "start": "http-server",
On 2018/02/22 19:07:28, saroyanm wrote:
> Suggestion: What about watching changes while running local server ?
> Any arguments against it ?
> I think it's handy when you don't need to run watcher every time separately.

IIRC you can have only one output in console through cmd1 & cmd2, which one you
prefer? I think having possible errors on build visible is key, and I need to
double check once you exit both processes are gone.

https://codereview.adblockplus.org/29705690/diff/29705808/package.json#newcode10
package.json:10: "bundle": "npm run bundle:desktop-options",
On 2018/02/22 19:07:28, saroyanm wrote:
> Detail: I think it will be helpful to document in README file how to create a
> bundle and run watchers.

it's a matter of copy and paste using a different entry point name but surely I
can try to explain it on the README.

https://codereview.adblockplus.org/29705690/diff/29705808/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 js/desktop-options.js >> ./desktop-options.js",
On 2018/02/22 18:54:32, saroyanm wrote:
> What does double ">>" mean in browserify, I thought the syntax is something
like
> "destination" > "target" ?

as explained by email, > and >> are not browserify features, rather how
shell/bash work.

[a.giammarchi, 2018-02-23 09:25:03]

added ^ on .hgignore + described both bundle and watch scripts.

Also both htt-server and watch are run on start and both output are shown in
console.

[saroyanm, 2018-02-23 11:26:17]

https://codereview.adblockplus.org/29705690/diff/29705808/package.json
File package.json (right):

https://codereview.adblockplus.org/29705690/diff/29705808/package.json#newcode9
package.json:9: "start": "http-server",
On 2018/02/23 08:51:37, a.giammarchi wrote:
> On 2018/02/22 19:07:28, saroyanm wrote:
> > Suggestion: What about watching changes while running local server ?
> > Any arguments against it ?
> > I think it's handy when you don't need to run watcher every time separately.
> 
> IIRC you can have only one output in console through cmd1 & cmd2, which one
you
> prefer? I think having possible errors on build visible is key, and I need to
> double check once you exit both processes are gone.

I agree that build is more important, I have couple of suggestions:
* Use "concurrently" https://www.npmjs.com/package/concurrently AFAIK it also
should care of exit
* Make watch during server run optional (this is irregardless of concurrently
usage)
* Push the way it was(run start and watch separately and revisit this question
later on).

All of options will work fine with me.

https://codereview.adblockplus.org/29705690/diff/29705808/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 js/desktop-options.js >> ./desktop-options.js",
On 2018/02/23 08:51:37, a.giammarchi wrote:
> On 2018/02/22 18:54:32, saroyanm wrote:
> > What does double ">>" mean in browserify, I thought the syntax is something
> like
> > "destination" > "target" ?
> 
> as explained by email, > and >> are not browserify features, rather how
> shell/bash work.

Acknowledged.

[a.giammarchi, 2018-02-23 12:02:11]

Answered Manvel

https://codereview.adblockplus.org/29705690/diff/29705808/package.json
File package.json (right):

https://codereview.adblockplus.org/29705690/diff/29705808/package.json#newcode9
package.json:9: "start": "http-server",
On 2018/02/23 11:26:16, saroyanm wrote:
> On 2018/02/23 08:51:37, a.giammarchi wrote:
> > On 2018/02/22 19:07:28, saroyanm wrote:
> > > Suggestion: What about watching changes while running local server ?
> > > Any arguments against it ?
> > > I think it's handy when you don't need to run watcher every time
separately.
> > 
> > IIRC you can have only one output in console through cmd1 & cmd2, which one
> you
> > prefer? I think having possible errors on build visible is key, and I need
to
> > double check once you exit both processes are gone.
> 
> I agree that build is more important, I have couple of suggestions:
> * Use "concurrently" https://www.npmjs.com/package/concurrently AFAIK it also
> should care of exit
> * Make watch during server run optional (this is irregardless of concurrently
> usage)
> * Push the way it was(run start and watch separately and revisit this question
> later on).
> 
> All of options will work fine with me.

current command works just fine and we don't need to handle kill-others cases so
I think we can go with this?

[saroyanm, 2018-02-23 12:12:58]

On 2018/02/23 12:02:11, a.giammarchi wrote:
> Answered Manvel
> 
> https://codereview.adblockplus.org/29705690/diff/29705808/package.json
> File package.json (right):
> 
>
https://codereview.adblockplus.org/29705690/diff/29705808/package.json#newcode9
> package.json:9: "start": "http-server",
> On 2018/02/23 11:26:16, saroyanm wrote:
> > On 2018/02/23 08:51:37, a.giammarchi wrote:
> > > On 2018/02/22 19:07:28, saroyanm wrote:
> > > > Suggestion: What about watching changes while running local server ?
> > > > Any arguments against it ?
> > > > I think it's handy when you don't need to run watcher every time
> separately.
> > > 
> > > IIRC you can have only one output in console through cmd1 & cmd2, which
one
> > you
> > > prefer? I think having possible errors on build visible is key, and I need
> to
> > > double check once you exit both processes are gone.
> > 
> > I agree that build is more important, I have couple of suggestions:
> > * Use "concurrently" https://www.npmjs.com/package/concurrently AFAIK it
also
> > should care of exit
> > * Make watch during server run optional (this is irregardless of
concurrently
> > usage)
> > * Push the way it was(run start and watch separately and revisit this
question
> > later on).
> > 
> > All of options will work fine with me.
> 
> current command works just fine and we don't need to handle kill-others cases
so
> I think we can go with this?

Agree, if we will notice issue we will update implementation. LGTM