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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years ago by a.giammarchi
Modified:
2 years 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
2 years 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. ...
2 years 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 ...
2 years ago (2018-02-22 16:42:04 UTC) #3
a.giammarchi
I have cleaned everything up.
2 years 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 ...
2 years 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 ...
2 years 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 ...
2 years 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: ...
2 years 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 ...
2 years 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 ...
2 years 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: ...
2 years ago (2018-02-23 12:02:11 UTC) #11
saroyanm
2 years 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