Issue 6104 - Minified code in help center repository

Issue 6104 - Minified code in help center repository

[ire, 2017-11-29 14:30:00]

Here's my first crack at this. A couple things to note:

- Since there were so many files, I didn't think it made sense to have the
"source" files in the same folder (particularly for the javascript files), so I
moved them to a new root `src` folder. This is just my suggestion for how I
think we should handle this, but I'm open to other suggestions you may have
- You mentioned in the issue that "Dependencies should be managed by npm and
excluded from source code repository", but I'm not sure how this would work for
files that aren't in npm, like most of the vendor javascript files

[juliandoucette, 2017-11-29 23:30:36]

> Since there were so many files, I didn't think it made sense to have the
> "source" files in the same folder (particularly for the javascript files), so
I
> moved them to a new root `src` folder. This is just my suggestion for how I
> think we should handle this, but I'm open to other suggestions you may have

I don't think that `src` makes sense in this context because all of the other
directories are also "source" directories.

> You mentioned in the issue that "Dependencies should be managed by npm and
> excluded from source code repository", but I'm not sure how this would work
for
> files that aren't in npm, like most of the vendor javascript files

Good question.

Short, not real, answer: 

bower? :D

Long, real answer: 

My thinking was that these are dependencies and therefore should be managed by
our package manager. We could install available npm packages or use source code
repositories as dependencies and copy the relevant files into the appropriate
directories via our build process (so that they are available in production).

I'm not sure about this though. It seems like a lot of hassle.

https://codereview.adblockplus.org/29624561/diff/29624562/.gitignore
File .gitignore (right):

https://codereview.adblockplus.org/29624561/diff/29624562/.gitignore#newcode2
.gitignore:2: static/css/*
I think that the "*" is unnecessary in both git and mercurial.

https://codereview.adblockplus.org/29624561/diff/29624562/gulpfile.js
File gulpfile.js (right):

https://codereview.adblockplus.org/29624561/diff/29624562/gulpfile.js#newcode1
gulpfile.js:1: /*!
Tabs! :O

https://codereview.adblockplus.org/29624561/diff/29624562/gulpfile.js#newcode25
gulpfile.js:25: * CSS
There seems to be something wrong with the SCSS source mapping. It only finds
variables.scss.

https://codereview.adblockplus.org/29624561/diff/29624562/gulpfile.js#newcode51
gulpfile.js:51: const minify = require('gulp-minify');
Requires usually go at the top

[ire, 2017-11-30 10:04:43]

New patchset uploaded

On 2017/11/29 23:30:36, juliandoucette wrote:
> > Since there were so many files, I didn't think it made sense to have the
> > "source" files in the same folder (particularly for the javascript files),
so
> I
> > moved them to a new root `src` folder. This is just my suggestion for how I
> > think we should handle this, but I'm open to other suggestions you may have
> 
> I don't think that `src` makes sense in this context because all of the other
> directories are also "source" directories.

Good point. What do you think of separating the `static` folder into two: `dist`
and `src`.

So for now we would have:

static
  - src
    - scss
    - js
  - dist
    - css
    - fonts
    - img
    - js

(In principal we shouldn't have anything in the dist folder that isn't generated
from the src folder, but we could work on build steps for the other types of
files later if this is what we want)

On 2017/11/29 23:30:36, juliandoucette wrote:
> > You mentioned in the issue that "Dependencies should be managed by npm and
> > excluded from source code repository", but I'm not sure how this would work
> for
> > files that aren't in npm, like most of the vendor javascript files
> 
> Good question.
> 
> Short, not real, answer: 
> 
> bower? :D
> 
> Long, real answer: 
> 
> My thinking was that these are dependencies and therefore should be managed by
> our package manager. We could install available npm packages or use source
code
> repositories as dependencies and copy the relevant files into the appropriate
> directories via our build process (so that they are available in production).
> 
> I'm not sure about this though. It seems like a lot of hassle.

I agree with you that, at least in principal, all dependencies should be managed
using a package manager. However, I do think that it is beneficial to have
third-party dependencies hosted ourselves, i.e. not dependent on their remote
repo being available. Particularly for polyfills like the ones we are mainly
using, where we likely won't be updating the scripts at all. I haven't come to a
strong decision on this myself, but my inclination is to leave things hosted the
way it is for now, unless we have a strong reason to change.

https://codereview.adblockplus.org/29624561/diff/29624562/.gitignore
File .gitignore (right):

https://codereview.adblockplus.org/29624561/diff/29624562/.gitignore#newcode2
.gitignore:2: static/css/*
On 2017/11/29 23:30:36, juliandoucette wrote:
> I think that the "*" is unnecessary in both git and mercurial.

Ack. Done.

https://codereview.adblockplus.org/29624561/diff/29624562/gulpfile.js
File gulpfile.js (right):

https://codereview.adblockplus.org/29624561/diff/29624562/gulpfile.js#newcode1
gulpfile.js:1: /*!
On 2017/11/29 23:30:36, juliandoucette wrote:
> Tabs! :O

Oops :O !

Fixed :)

https://codereview.adblockplus.org/29624561/diff/29624562/gulpfile.js#newcode25
gulpfile.js:25: * CSS
On 2017/11/29 23:30:36, juliandoucette wrote:
> There seems to be something wrong with the SCSS source mapping. It only finds
> variables.scss.

Fixed. It was because I was calling sourcemaps.write('./') twice, which it
appears you should only do once, So I had to separate the tasks for the default
and minified CSS versions

https://codereview.adblockplus.org/29624561/diff/29624562/gulpfile.js#newcode51
gulpfile.js:51: const minify = require('gulp-minify');
On 2017/11/29 23:30:36, juliandoucette wrote:
> Requires usually go at the top

Is that a coding style thing? I thought it made sense to separate them by each
task

[juliandoucette, 2017-12-01 15:53:26]

> Good point. What do you think of separating the `static` folder into two:
`dist`
> and `src`.
> 
> So for now we would have:
> 
> static
>     - src
>     - scss
>     - js
>   - dist
>     - css
>     - fonts
>     - img
>     - js

I think this makes sense.

> (In principal we shouldn't have anything in the dist folder that isn't
generated
> from the src folder, but we could work on build steps for the other types of
> files later if this is what we want)

These were my thoughts aswell.

> I agree with you that, at least in principal, all dependencies should be
managed
> using a package manager. However, I do think that it is beneficial to have
> third-party dependencies hosted ourselves, i.e. not dependent on their remote
> repo being available. Particularly for polyfills like the ones we are mainly
> using, where we likely won't be updating the scripts at all. I haven't come to
a
> strong decision on this myself, but my inclination is to leave things hosted
the
> way it is for now, unless we have a strong reason to change.

I agree.

---

Detail: You haven't implemented the src/dist directory structure in the latest
PatchSet.

[ire, 2017-12-04 08:41:03]

On 2017/12/01 15:53:26, juliandoucette wrote:
> Detail: You haven't implemented the src/dist directory structure in the latest
> PatchSet.

Done.

[juliandoucette, 2017-12-04 10:49:38]

I checked with kzar and palant in #adblockplus  (they responded this morning if
you want to read the whole conversation).

https://codereview.adblockplus.org/29624561/diff/29624562/gulpfile.js
File gulpfile.js (right):

https://codereview.adblockplus.org/29624561/diff/29624562/gulpfile.js#newcode51
gulpfile.js:51: const minify = require('gulp-minify');
On 2017/11/30 10:04:43, ire wrote:
> On 2017/11/29 23:30:36, juliandoucette wrote:
> > Requires usually go at the top
> 
> Is that a coding style thing? I thought it made sense to separate them by each
> task

"<kzar> julian: Yea. It's certainly a convention to generally require stuff at
the top of the file, but not a rule as far as I know."

I guess it's not a code style rule because their are circumstances where it
makes sense e.g. conditional requires and circular dependencies. I think that we
should keep them at the top for the sake of consistency.

[ire, 2017-12-04 14:20:03]

On 2017/12/04 10:49:38, juliandoucette wrote:
> I checked with kzar and palant in #adblockplus  (they responded this morning
if
> you want to read the whole conversation).
> 
> https://codereview.adblockplus.org/29624561/diff/29624562/gulpfile.js
> File gulpfile.js (right):
> 
>
https://codereview.adblockplus.org/29624561/diff/29624562/gulpfile.js#newcode51
> gulpfile.js:51: const minify = require('gulp-minify');
> On 2017/11/30 10:04:43, ire wrote:
> > On 2017/11/29 23:30:36, juliandoucette wrote:
> > > Requires usually go at the top
> > 
> > Is that a coding style thing? I thought it made sense to separate them by
each
> > task
> 
> "<kzar> julian: Yea. It's certainly a convention to generally require stuff at
> the top of the file, but not a rule as far as I know."
> 
> I guess it's not a code style rule because their are circumstances where it
> makes sense e.g. conditional requires and circular dependencies. I think that
we
> should keep them at the top for the sake of consistency.

Alright, done!

[juliandoucette, 2017-12-06 15:01:38]

I only reviewed the gulpfile.

https://codereview.adblockplus.org/29624561/diff/29629921/.gitignore
File .gitignore (right):

https://codereview.adblockplus.org/29624561/diff/29629921/.gitignore#newcode2
.gitignore:2: static/dist/css
(Commented here because I can't comment on an empty file)

Please ignore .DS_Store globally (not in this project) and exclude it from the
next PatchSet.

https://codereview.adblockplus.org/29624561/diff/29629921/gulpfile.js
File gulpfile.js (right):

https://codereview.adblockplus.org/29624561/diff/29629921/gulpfile.js#newcode34
gulpfile.js:34: const entryScssFile = 'main';
NIT: Discussed in-person. I think this is unnecessary/overdeveloped.

https://codereview.adblockplus.org/29624561/diff/29629921/gulpfile.js#newcode38
gulpfile.js:38: /* Expanded CSS
NIT: It seems like this comment describes the task below, not a category of
tasks below. If the comment is necessary, then I suggest using a jsdoc comment
instead.

https://codereview.adblockplus.org/29624561/diff/29629921/gulpfile.js#newcode41
gulpfile.js:41: gulp.task('css-default', function() {
I don't think that it's necessary to output both minified/not versions of our
CSS if we have sourcemaps enabled.

https://codereview.adblockplus.org/29624561/diff/29629921/gulpfile.js#newcode67
gulpfile.js:67: gulp.task('js', function() {
It seems like we should be able to combine the src -> dist vendor/not tasks?

https://codereview.adblockplus.org/29624561/diff/29629921/gulpfile.js#newcode95
gulpfile.js:95: gulp.watch('./static/src/js/*.js', ['js']);
You could watch './static/src/js/**/*.js' instead of *.js && vendor/*.js

[ire, 2017-12-07 14:54:25]

https://codereview.adblockplus.org/29624561/diff/29629921/.gitignore
File .gitignore (right):

https://codereview.adblockplus.org/29624561/diff/29629921/.gitignore#newcode2
.gitignore:2: static/dist/css
On 2017/12/06 15:01:34, juliandoucette wrote:
> (Commented here because I can't comment on an empty file)
> 
> Please ignore .DS_Store globally (not in this project) and exclude it from the
> next PatchSet.

Done.

https://codereview.adblockplus.org/29624561/diff/29629921/gulpfile.js
File gulpfile.js (right):

https://codereview.adblockplus.org/29624561/diff/29629921/gulpfile.js#newcode34
gulpfile.js:34: const entryScssFile = 'main';
On 2017/12/06 15:01:36, juliandoucette wrote:
> NIT: Discussed in-person. I think this is unnecessary/overdeveloped.

Done.

https://codereview.adblockplus.org/29624561/diff/29629921/gulpfile.js#newcode38
gulpfile.js:38: /* Expanded CSS
On 2017/12/06 15:01:36, juliandoucette wrote:
> NIT: It seems like this comment describes the task below, not a category of
> tasks below. If the comment is necessary, then I suggest using a jsdoc comment
> instead.

Ack. This isn't necessary anymore since we are only having the minified file

https://codereview.adblockplus.org/29624561/diff/29629921/gulpfile.js#newcode41
gulpfile.js:41: gulp.task('css-default', function() {
On 2017/12/06 15:01:35, juliandoucette wrote:
> I don't think that it's necessary to output both minified/not versions of our
> CSS if we have sourcemaps enabled.

Done.

https://codereview.adblockplus.org/29624561/diff/29629921/gulpfile.js#newcode67
gulpfile.js:67: gulp.task('js', function() {
On 2017/12/06 15:01:34, juliandoucette wrote:
> It seems like we should be able to combine the src -> dist vendor/not tasks?

You're right. I didn't think it would preserve the vendor directory, but looks
like it does.

Done.

https://codereview.adblockplus.org/29624561/diff/29629921/gulpfile.js#newcode95
gulpfile.js:95: gulp.watch('./static/src/js/*.js', ['js']);
On 2017/12/06 15:01:35, juliandoucette wrote:
> You could watch './static/src/js/**/*.js' instead of *.js && vendor/*.js

Done.

[juliandoucette, 2017-12-08 14:57:24]

LGTM + NITs

https://codereview.adblockplus.org/29624561/diff/29632976/gulpfile.js
File gulpfile.js (right):

https://codereview.adblockplus.org/29624561/diff/29632976/gulpfile.js#newcode33
gulpfile.js:33: gulp.task('css', function() {
Single quotes :( ...

(I love single quotes... I want to use them someday... But... You know.)

https://codereview.adblockplus.org/29624561/diff/29632976/gulpfile.js#newcode34
gulpfile.js:34: return gulp.src(`./static/src/scss/main.scss`)
Backtics...

https://codereview.adblockplus.org/29624561/diff/29632976/gulpfile.js#newcode52
gulpfile.js:52: ext: {src:'.js', min:'.min.js'},
Maybe src:js is unnecessary?

[ire, 2017-12-11 15:29:34]

https://codereview.adblockplus.org/29624561/diff/29632976/gulpfile.js
File gulpfile.js (right):

https://codereview.adblockplus.org/29624561/diff/29632976/gulpfile.js#newcode33
gulpfile.js:33: gulp.task('css', function() {
On 2017/12/08 14:57:21, juliandoucette wrote:
> Single quotes :( ...
> 
> (I love single quotes... I want to use them someday... But... You know.)

Done.

https://codereview.adblockplus.org/29624561/diff/29632976/gulpfile.js#newcode34
gulpfile.js:34: return gulp.src(`./static/src/scss/main.scss`)
On 2017/12/08 14:57:17, juliandoucette wrote:
> Backtics...

Done.

https://codereview.adblockplus.org/29624561/diff/29632976/gulpfile.js#newcode52
gulpfile.js:52: ext: {src:'.js', min:'.min.js'},
On 2017/12/08 14:57:17, juliandoucette wrote:
> Maybe src:js is unnecessary? 

Done.