Michał Gołębiowski-Owczarek [:mgol] Assignee
	Description • 11 years ago

It would be nice if l20n.js used Grunt for compiling instead of Makefile. It would also help with automating tests, could be connected to Travis on GitHub pull requests and would make a wide selection of Grunt tasks available for l20n to use.

Would you be interested in such a switch? Maybe I'd find time one day to do this implementation is you're interested.

	Michał Gołębiowski-Owczarek [:mgol] Assignee
	Comment 1 • 11 years ago

Together with the rewrite it would be possible to integrate jsHint into the workflow which could prevent most common JS errors and lower the burden on pull request reviewers for basic linting tasks. I'd need a simple code guideline for it to work, though.

	Staś Małolepszy :stas
	Comment 2 • 11 years ago

I was looking at grunt, but didn't have time to learn it.  That's why I went with Makefiles.  It looks promising.

Just as an inventory of things that we have right now:  we are hooked up to Travis: https://travis-ci.org/l20n/l20n.js (bug 881704) and we also use gjslint (bug 883100).  Are there benefits beyond what we have right now related to these two areas?

Regarding pull requests, I'm not so much interested in integration with github because I believe that all discussions and code reviews should happen in tools-l10n and bugzilla.  Think 10 years from now, I'm sure Bugzilla will be around.  Github?  We'll see. (I hope they will and I wish them well.)

That said, I'd still like to move away from Makefile.  It introduces an unneeded dependency which is hard to meet on Windows.  And really, Makefiles, being declarative by design, don't really fit into what I had shoehorned ours into:  a list of tasks.  My understanding is that Grunt is a much better suited tool for this.

It would be great if you could prepare a patch! :)

	Michał Gołębiowski-Owczarek [:mgol] Assignee
	Comment 3 • 11 years ago

Yeah, Grunt is all about tasks and it's written in node which should help with compiling on Windows (and thus it'd lower the commiter entry barrier).

Grunt should simplify some tasks (like wrapping UglifyJS). As for the linting, I'm not terribly faimiliar with gjslint, I use jshint for everything (nice bonus: it's written in JavaScript ;)(, jsHint has lots of configuration options which is nice. I thought about jsHint when I noticed you have lots of trailing whitespaces in the codebase, it can bite sometimes. :)

As for tests, I'd try to use Karma with its grunt-karma task which lets you run tests in real browsers. Karma can be integrated with Jenkins & Travis. It also has an autoWatch mode which runs tests live when developing.

Lots of things to improve!

	Staś Małolepszy :stas
	Comment 4 • 11 years ago

As per the discussion on IRC, assigning to Michał.

(In reply to Michał Gołębiowski from comment #3)

> Grunt should simplify some tasks (like wrapping UglifyJS). As for the
> linting, I'm not terribly faimiliar with gjslint, I use jshint for
> everything (nice bonus: it's written in JavaScript ;)(, jsHint has lots of
> configuration options which is nice. I thought about jsHint when I noticed
> you have lots of trailing whitespaces in the codebase, it can bite
> sometimes. :)
> 
> As for tests, I'd try to use Karma with its grunt-karma task which lets you
> run tests in real browsers. Karma can be integrated with Jenkins & Travis.
> It also has an autoWatch mode which runs tests live when developing.

I love the sound of both of these things.  Let's file separate bugs for each and fix them independently of this one.
 
> Lots of things to improve!

Yeah, definitely!

	Michał Gołębiowski-Owczarek [:mgol] Assignee
	Comment 5 • 11 years ago

After applying the patch and invoking `npm install`, to:

1) build the project and test it, invoke: `grunt`
2) build the project without testing, invoke: `grunt build`
3) Instead of `make perf`, `make reference`, `make coverage`, `make test` use `grunt perf`, `grunt reference`, `grunt coverage`, `grunt test` respectively
4) To re-build the project and re-run tests after every change, do: `grunt build && grunt watch` (`grunt server` should work like that but there's a problem with that, I added a TODO comment)

	Staś Małolepszy :stas
	Comment 6 • 11 years ago

Comment on attachment 807626 [details] [diff] [review]
Switch from Makefile to Grunt and from gjslint to jsHint.

Review of attachment 807626 [details] [diff] [review]:
-----------------------------------------------------------------

This. Is. Great.

Thanks so much, Michał!  This change will greatly improve the quality and consistency of the codebase.  I'm also happy to remove the make dependency.  Good stuff.

r=me, with comments below.  Please attach a new patch with appropriate changes if you agree with my comments, and I'll land it for you.

::: .jshintrc
@@ +3,5 @@
> +  "boss": true,
> +  "curly": true,
> +  "eqeqeq": true,
> +  "eqnull": true,
> +  "forin": false,

Maybe switch forin to true?

@@ +21,5 @@
> +
> +  "quotmark": "single",
> +  "maxcomplexity": 20,
> +  "maxerr": 50,
> +  "maxlen": 120,

This should be 80.  I know most of the test files fail this.  It looks it would be possible to put 80 here and 120 in tests/.jshintrc?

::: Gruntfile.js
@@ +87,5 @@
> +
> +  grunt.registerTask('test', ['mochaTest:dot']);
> +
> +  // TODO first make sure `dist/docs` exists.
> +  grunt.registerTask('coverage', function () {

Grunt complains that jscoverage is not found.  I think we should add it to devDependencies in package.json and maybe you'll need to specify the path to ./node_modules/.bin/jscoverage somehwere?

::: Makefile
@@ -19,5 @@
> -  tests/lib/context/*.js \
> -  tests/lib/compiler/*.js \
> -  tests/integration/*.js
> -ifeq ($(INSECURE), 1)
> -LIB_FILES += tests/lib/compiler/insecure/*.js

Is there a way to add something similar in the mocha task?

::: bindings/.jshintrc
@@ +1,1 @@
> +../lib/.jshintrc
\ No newline at end of file

Why do we need a separate .jshintrc file here and in lib?  Isn't the one in the root dir enough?

::: docs/build.md
@@ +21,4 @@
>      git clone https://github.com/l20n/l20n.js.git
>      cd l20n.js
>      npm install
> +    grunt

Can you add something like:

  .. which is an alias for: clean compile:html uglify:html test

BTW it looks like currently `grunt` doesn't test, just cleans and builds.  Can we add testing as well?

::: grunt/config/mocha-test.js
@@ +12,5 @@
> +module.exports = {
> +  dot: {
> +    options: {
> +      reporter: 'dot',
> +      require: 'should',

Is it possible to turn on mocha's growl notifications?  I find it more helpful to see "1 test failed" than "Grunt task mochaTest:dot failed"

::: grunt/tasks/compile.js
@@ +34,5 @@
> +          path.join(__dirname, '..', '..', 'bindings') ,
> +          path.join(__dirname, '..', '..', 'lib', 'client') ,
> +          path.join(__dirname, '..', '..', 'lib'),
> +          __dirname  // look for dummy amdefine in build/ to make
> +          // the output prettier (bug 869210)

The last element here needs to be changed as well to suppress warnings about amdefine not being found:

          // Bug 869210: look for dummy amdefine in build/ to make the output 
          // prettier.  It will be removed by the removeAmdefine filter.
          path.join(__dirname, '..', '..', 'build')

::: lib/l20n/compiler.js
@@ +146,5 @@
>    var define = require('amdefine')(module);
>  }
> +define(function (require, exports) {
> +  // TODO change newcap to true?
> +  /* jshint strict: false, newcap: false */

I guess this is because of the "ctor" and "constructor" which are assigned dynamically?  I see that you changed them to Ctor and Constructor respectively, which is fine to me.  Can we remove this comment then?

@@ +428,2 @@
>        var name = node.name;
> +      return function identifier(locals) {

Oh, great, I never got around to removing those unneeded "entry" and "ctxdata" args). Thanks!

@@ +472,2 @@
>          try {
> +          value = _globals[name].get();

Any reason to define value outside of try-catch?  It's still hoisted and undefined in the scope of the function, isn't it?

@@ +508,3 @@
>              return [locals, parsed.content];
>            }
> +          complex = new Expression(parsed, entry);

This should be = Expression, not = new Expression.  Expression is just a dispatcher function, not a constructor.  (All expressions are functions, not Object-like objects.)

::: lib/l20n/intl.js
@@ +383,4 @@
>     * Spec: ECMAScript Internationalization API Specification, 9.2.2.
>     * Spec: RFC 4647, section 3.4.
>     */
> +  function getBestAvailableLocale(availableLocales, locale) {

All code in this file except the addition of PrioritizeLocales comes from Firefox's implementation of http://www.ecma-international.org/ecma-402/1.0/.

The spec defines the names of the methods, e.g. BestAvailableLocale.

It might make sense to leave this file as-is.  Can we exclude it completely from jshint?

::: package.json
@@ +63,3 @@
>    },
>    "engine": {
>      "node": ">=0.10"

Some grunt dependencies, like uglify and grunt-notify, don't work with node 0.11 right now.  Should we maybe reflect that here?

	Michał Gołębiowski-Owczarek [:mgol] Assignee
	Comment 7 • 11 years ago

(In reply to Staś Małolepszy :stas from comment #6)
> Comment on attachment 807626 [details] [diff] [review]
> Switch from Makefile to Grunt and from gjslint to jsHint.
> 
> Review of attachment 807626 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This. Is. Great.
> 
> Thanks so much, Michał!  This change will greatly improve the quality and
> consistency of the codebase.  I'm also happy to remove the make dependency. 
> Good stuff.
> 
> r=me, with comments below.  Please attach a new patch with appropriate
> changes if you agree with my comments, and I'll land it for you.
> 
> ::: .jshintrc
> @@ +3,5 @@
> > +  "boss": true,
> > +  "curly": true,
> > +  "eqeqeq": true,
> > +  "eqnull": true,
> > +  "forin": false,
> 
> Maybe switch forin to true?

I can do it but I suppose I'd need to filter `for key in obj` results by `obj.hasOwnProperty(key)` then which may influence performance a little if used in more intense loops. jQuery has `forin` set to false, we don't officially support extending Object.prototype which would be a main cause of problems (another one would be extending Array.prototype).

But if you want to toggle the switch, I can do it. :) (I'm leaving it as it is until
you reply).

> 
> @@ +21,5 @@
> > +
> > +  "quotmark": "single",
> > +  "maxcomplexity": 20,
> > +  "maxerr": 50,
> > +  "maxlen": 120,
> 
> This should be 80.  I know most of the test files fail this.  It looks it
> would be possible to put 80 here and 120 in tests/.jshintrc?

Sure, will do. We can switch tests to 80 in subsequent commits since that will require some wrapping changes.

I also thought about lowering maxcomplexity to 10 but there are places where this fails currently and I didn't want to make major changes to code structure in this patch.

> 
> ::: Gruntfile.js
> @@ +87,5 @@
> > +
> > +  grunt.registerTask('test', ['mochaTest:dot']);
> > +
> > +  // TODO first make sure `dist/docs` exists.
> > +  grunt.registerTask('coverage', function () {
> 
> Grunt complains that jscoverage is not found.  I think we should add it to
> devDependencies in package.json and maybe you'll need to specify the path to
> ./node_modules/.bin/jscoverage somehwere?

Could you remove the whole node_modules directory, re-run `npm install` and try again? I can't reproduce it...

> 
> ::: Makefile
> @@ -19,5 @@
> > -  tests/lib/context/*.js \
> > -  tests/lib/compiler/*.js \
> > -  tests/integration/*.js
> > -ifeq ($(INSECURE), 1)
> > -LIB_FILES += tests/lib/compiler/insecure/*.js
> 
> Is there a way to add something similar in the mocha task?

Done. ;) (but try to test it, sth like `INSECURE=1 grunt test` should work)

> 
> ::: bindings/.jshintrc
> @@ +1,1 @@
> > +../lib/.jshintrc
> \ No newline at end of file
> 
> Why do we need a separate .jshintrc file here and in lib?  Isn't the one in
> the root dir enough?

The one in the root dir is for JS files in node, the ones in lib or bindings
are executed in the browser environment (hence node: true and browser: true
options). This defines additional globals and other minor changes.

The cleanest option would be to keep a JSON with shared options somewhere
and dynamically append other options to in in jshint configuration. The
huge drawback is that IDEs completely don't know how to deal with this situation -
with how it is now it's easy to see jsHint errors in the code even without running
grunt jshint or grunt test if one uses one of many editors with jshint support
built-in or available via plugin (WebStorm, Sublime Text, Emacs, etc.).

I guess it's possible to put such JSON somewhere and have proper .jshintrc files
generated automatically via a task (those files would be in .gitignore) so that
IDEs won't get lost. :)

> 
> ::: docs/build.md
> @@ +21,4 @@
> >      git clone https://github.com/l20n/l20n.js.git
> >      cd l20n.js
> >      npm install
> > +    grunt
> 
> Can you add something like:
> 
>   .. which is an alias for: clean compile:html uglify:html test

But that's what `grunt build` is aliased to, `grunt` does much more. :)

> 
> BTW it looks like currently `grunt` doesn't test, just cleans and builds. 
> Can we add testing as well?

Added. I intended to do that and I forgot.

> 
> ::: grunt/config/mocha-test.js
> @@ +12,5 @@
> > +module.exports = {
> > +  dot: {
> > +    options: {
> > +      reporter: 'dot',
> > +      require: 'should',
> 
> Is it possible to turn on mocha's growl notifications?  I find it more
> helpful to see "1 test failed" than "Grunt task mochaTest:dot failed"

I'd have to look more into that. Could we try to add it separately?

> 
> ::: grunt/tasks/compile.js
> @@ +34,5 @@
> > +          path.join(__dirname, '..', '..', 'bindings') ,
> > +          path.join(__dirname, '..', '..', 'lib', 'client') ,
> > +          path.join(__dirname, '..', '..', 'lib'),
> > +          __dirname  // look for dummy amdefine in build/ to make
> > +          // the output prettier (bug 869210)
> 
> The last element here needs to be changed as well to suppress warnings about
> amdefine not being found:
> 
>           // Bug 869210: look for dummy amdefine in build/ to make the
> output 
>           // prettier.  It will be removed by the removeAmdefine filter.
>           path.join(__dirname, '..', '..', 'build')
> 
> ::: lib/l20n/compiler.js
> @@ +146,5 @@
> >    var define = require('amdefine')(module);
> >  }
> > +define(function (require, exports) {
> > +  // TODO change newcap to true?
> > +  /* jshint strict: false, newcap: false */
> 
> I guess this is because of the "ctor" and "constructor" which are assigned
> dynamically?  I see that you changed them to Ctor and Constructor
> respectively, which is fine to me.  Can we remove this comment then?

Actually, there are more things to change. There are many places where
`Expression` and `IndexExpression` is used as a usual function, without the
`new` keyword. Both adding the keyword or changing the names to `expression`
and `indexExpression` produced many errors in tests so I left it for now...

> 
> @@ +428,2 @@
> >        var name = node.name;
> > +      return function identifier(locals) {
> 
> Oh, great, I never got around to removing those unneeded "entry" and
> "ctxdata" args). Thanks!
> 
> @@ +472,2 @@
> >          try {
> > +          value = _globals[name].get();
> 
> Any reason to define value outside of try-catch?  It's still hoisted and
> undefined in the scope of the function, isn't it?

It is, though jsHint warns about that. I also find it to be a more clear approach
as making a declaration in the parems might suggest it's block-scoped.
There is another advantage of declaring it in this way, though. One day, when
all browsers you handle support ES6 let & const, this kind of code will translate
to them in a more straightforward way, whereas the previous approach would cause
a lot of var -> const or let substitutions to make tests fail.

> 
> @@ +508,3 @@
> >              return [locals, parsed.content];
> >            }
> > +          complex = new Expression(parsed, entry);
> 
> This should be = Expression, not = new Expression.  Expression is just a
> dispatcher function, not a constructor.  (All expressions are functions, not
> Object-like objects.)

True, I wanted tor get rid of the `newcap: true` declaration here (without success)
and I forgot to revert this one change. Changed back.

> 
> ::: lib/l20n/intl.js
> @@ +383,4 @@
> >     * Spec: ECMAScript Internationalization API Specification, 9.2.2.
> >     * Spec: RFC 4647, section 3.4.
> >     */
> > +  function getBestAvailableLocale(availableLocales, locale) {
> 
> All code in this file except the addition of PrioritizeLocales comes from
> Firefox's implementation of http://www.ecma-international.org/ecma-402/1.0/.
> 
> The spec defines the names of the methods, e.g. BestAvailableLocale.
> 
> It might make sense to leave this file as-is.  Can we exclude it completely
> from jshint?

Sure, .jshintignore is for that. :) Done.
Do you want me to restore this file fully to the state it's on master currently
or just to change this name back?

> 
> ::: package.json
> @@ +63,3 @@
> >    },
> >    "engine": {
> >      "node": ">=0.10"
> 
> Some grunt dependencies, like uglify and grunt-notify, don't work with node
> 0.11 right now.  Should we maybe reflect that here?

I didn't really think about it, nearly every package.json I've seen used `>=` and
I don't even know the reasons. :) Done.

	Staś Małolepszy :stas
	Comment 8 • 11 years ago

(In reply to Michał Gołębiowski from comment #7)

> > Maybe switch forin to true?
> 
> I can do it but I suppose I'd need to filter `for key in obj` results by
> `obj.hasOwnProperty(key)` then which may influence performance a little if
> used in more intense loops. jQuery has `forin` set to false, we don't
> officially support extending Object.prototype which would be a main cause of
> problems (another one would be extending Array.prototype).

I forgot that bug 912469 hasn't landed yet :)  Let's land this with forin: false, and I'll change it to true in bug 912469.

> 
> > 
> > ::: Makefile
> > @@ -19,5 @@
> > > -  tests/lib/context/*.js \
> > > -  tests/lib/compiler/*.js \
> > > -  tests/integration/*.js
> > > -ifeq ($(INSECURE), 1)
> > > -LIB_FILES += tests/lib/compiler/insecure/*.js
> > 
> > Is there a way to add something similar in the mocha task?
> 
> Done. ;) (but try to test it, sth like `INSECURE=1 grunt test` should work)

Great, it works, thanks!

> 
> > 
> > ::: bindings/.jshintrc
> > @@ +1,1 @@
> > > +../lib/.jshintrc
> > \ No newline at end of file
> > 
> > Why do we need a separate .jshintrc file here and in lib?  Isn't the one in
> > the root dir enough?
> 
> The one in the root dir is for JS files in node, the ones in lib or bindings
> are executed in the browser environment (hence node: true and browser: true
> options). This defines additional globals and other minor changes.
> 
> The cleanest option would be to keep a JSON with shared options somewhere
> and dynamically append other options to in in jshint configuration. The
> huge drawback is that IDEs completely don't know how to deal with this
> situation - with how it is now it's easy to see jsHint errors in the code even without
> running grunt jshint or grunt test if one uses one of many editors with jshint
> support built-in or available via plugin (WebStorm, Sublime Text, Emacs, etc.).
> 
> I guess it's possible to put such JSON somewhere and have proper .jshintrc
> files  generated automatically via a task (those files would be in .gitignore) so
> that IDEs won't get lost. :)

Looks like the best option is to just leave them duplicated for now

> > Can you add something like:
> > 
> >   .. which is an alias for: clean compile:html uglify:html test
> 
> But that's what `grunt build` is aliased to, `grunt` does much more. :)

I didn't realize  Can you document what `grunt` deos then, please?

> > Is it possible to turn on mocha's growl notifications?  I find it more
> > helpful to see "1 test failed" than "Grunt task mochaTest:dot failed"
> 
> I'd have to look more into that. Could we try to add it separately?

Yeah.

> > I guess this is because of the "ctor" and "constructor" which are assigned
> > dynamically?  I see that you changed them to Ctor and Constructor
> > respectively, which is fine to me.  Can we remove this comment then?
> 
> Actually, there are more things to change. There are many places where
> `Expression` and `IndexExpression` is used as a usual function, without the
> `new` keyword. Both adding the keyword or changing the names to `expression`
> and `indexExpression` produced many errors in tests so I left it for now...

There shouldn't be any "new Expression" or "new IndexExpression" in the code.  I'll fix them, if there are.


> > Any reason to define value outside of try-catch?  It's still hoisted and
> > undefined in the scope of the function, isn't it?
> 
> It is, though jsHint warns about that. I also find it to be a more clear
> approach
> as making a declaration in the parems might suggest it's block-scoped.
> There is another advantage of declaring it in this way, though. One day, when
> all browsers you handle support ES6 let & const, this kind of code will
> translate to them in a more straightforward way, whereas the previous approach would
> cause a lot of var -> const or let substitutions to make tests fail.

OK, I'm not a big fan of this, but it's just a personal preference.  Let's go with your change.


> Do you want me to restore this file fully to the state it's on master
> currently or just to change this name back?

Let's restore it and if needed, we'll take care of it in a separate bug.

	Staś Małolepszy :stas
	Comment 14 • 11 years ago

Comment on attachment 807748 [details] [diff] [review]
Update grunt-docco3 to the version without the terminating bug

Review of attachment 807748 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work! r=me

	Staś Małolepszy :stas
	Comment 15 • 11 years ago

Landed on master: https://github.com/l20n/l20n.js/commit/7d6c65c3377a06ba0345ce098c120d9366c43583

\o/

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 16 • 11 years ago

I encounter several issues after the switch:

1) There is no shell target to build/uglify
2) The grunt shell:perf doesn't work with JSSHELL

That makes performance testing against jsshell impossible.

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 17 • 11 years ago

3) Also, the dist files don't work, it seems that the define() for each module doesn't get a name assigned, and cannot be loaded with require().

	Staś Małolepszy :stas
	Comment 18 • 11 years ago

Thanks for the feedback, Gandalf, I wish I had caught that before.  I backed out the landing for now and I'll try to fix these issues on Monday:

https://github.com/l20n/l20n.js/commit/4466a056dbb9da9f87c0b8bbf4062b703b92d3a4

	Staś Małolepszy :stas
	Comment 19 • 11 years ago

This adds grunt compile:shell and fixes JSSHELL=path/to/jsshell grunt perf.  
It's a diff ontop of Michał's last patch.

While the above grunt tasks appear to work now, the dist files are still 
broken.  This is due to the fact that, as Gandalf noted, the modules in the 
dist files aren't named and I'm not sure why.  Still working on this.

	Michał Gołębiowski-Owczarek [:mgol] Assignee
	Comment 20 • 11 years ago

Comment on attachment 808453 [details] [diff] [review]
Fix JSSHELL, add 'shell' compile target

Review of attachment 808453 [details] [diff] [review]:
-----------------------------------------------------------------

>+    var root = path.join.bind(path, __dirname, '..', '..');

Good idea. :)

The patch seems good (I see I made a couple of stupid mistakes) so if it works as intended, great!

	Staś Małolepszy :stas
	Comment 21 • 11 years ago

This should fix all the issues Gandalf listed.  The problem was with Dryice and 
the way it named modules.  I submitted a pull request upstream here:

    https://github.com/joewalker/dryice/pull/2

In the meantime, we can use my fork:

    https://github.com/stasm/dryice/tree/l20n

	Staś Małolepszy :stas
	Comment 22 • 11 years ago

Re-landed on master: https://github.com/l20n/l20n.js/commit/d8e28bbe972809d586f1cfe80fd022907b648df7

Please reopen if there are any critical issues still present, or file follow-up bugs for minor ones.

Thanks again, Michał!

	Michał Gołębiowski-Owczarek [:mgol] Assignee
	Comment 23 • 11 years ago

(In reply to Staś Małolepszy :stas from comment #22)
> Re-landed on master:
> https://github.com/l20n/l20n.js/commit/
> d8e28bbe972809d586f1cfe80fd022907b648df7
> 
> Please reopen if there are any critical issues still present, or file
> follow-up bugs for minor ones.
> 
> Thanks again, Michał!

Can you create a ticket about the `docco` task failures and link to it here? I'm not able to reproduce it but it'd be lovely to know how common this problem is, if it depends on OS, if it fails in just-cloned repo copies etc.

	Staś Małolepszy :stas
	Comment 24 • 11 years ago

(In reply to Michał Gołębiowski from comment #23)
> Can you create a ticket about the `docco` task failures and link to it here?
> I'm not able to reproduce it but it'd be lovely to know how common this
> problem is, if it depends on OS, if it fails in just-cloned repo copies etc.

Bug 919648.