Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(385)

Issue 29705690: Issue 6310 - Start using JavaScript modularization tool (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 9 months ago by a.giammarchi
Modified:
1 year, 9 months ago
Visibility:
Public.

Description

Issue 6310 - Start using JavaScript modularization tool

Patch Set 1 : #

Total comments: 11

Patch Set 2 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1381 lines, -1317 lines) Patch
M .hgignore View 1 1 chunk +1 line, -0 lines 0 comments Download
M README.md View 1 2 chunks +58 lines, -0 lines 0 comments Download
M js/desktop-options.js View 1 chunk +1313 lines, -1315 lines 0 comments Download
M package.json View 1 1 chunk +9 lines, -2 lines 0 comments Download

Messages

Total messages: 12
saroyanm
1 year, 9 months ago (2018-02-22 16:15:39 UTC) #1
Thomas Greiner
I think this is a good approach to introduce modules with minimal amount of side-effects. ...
1 year, 9 months ago (2018-02-22 16:24:18 UTC) #2
a.giammarchi
On 2018/02/22 16:24:18, Thomas Greiner wrote: > or do you intend to apply this to ...
1 year, 9 months ago (2018-02-22 16:42:04 UTC) #3
a.giammarchi
I have cleaned everything up.
1 year, 9 months ago (2018-02-22 17:23:42 UTC) #4
Thomas Greiner
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 ...
1 year, 9 months ago (2018-02-22 18:29:23 UTC) #5
saroyanm
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 ...
1 year, 9 months ago (2018-02-22 18:54:32 UTC) #6
saroyanm
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 ...
1 year, 9 months ago (2018-02-22 19:07:28 UTC) #7
a.giammarchi
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: ...
1 year, 9 months ago (2018-02-23 08:51:38 UTC) #8
a.giammarchi
added ^ on .hgignore + described both bundle and watch scripts. Also both htt-server and ...
1 year, 9 months ago (2018-02-23 09:25:03 UTC) #9
saroyanm
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 ...
1 year, 9 months ago (2018-02-23 11:26:17 UTC) #10
a.giammarchi
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: ...
1 year, 9 months ago (2018-02-23 12:02:11 UTC) #11
saroyanm
1 year, 9 months ago (2018-02-23 12:12:58 UTC) #12
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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5