search

inveniosoftware / invenio

Invenio digital library framework
https://invenio.readthedocs.io
MIT License
625 stars 292 forks source link

RFC: JavaScript Code Style #1769

Open invenio-developers opened 10 years ago

invenio-developers commented 10 years ago

Issue by jirikuncar Thursday May 22, 2014 at 11:54 GMT Originally opened as https://github.com/jirikuncar/invenio/issues/262

Revision Date Author
Initial 2014-05-22 @jirikuncar

Idea

It'd be nice to use the standard way for JavaScript.

Proposals

invenio-developers commented 10 years ago

Comment by lnielsen-cern Thursday May 22, 2014 at 11:57 GMT

See previous discussion as well http://invenio-software.org/wiki/Talk/JSFrameworks

invenio-developers commented 10 years ago

Comment by kneczaj Thursday May 22, 2014 at 12:13 GMT

For me https://github.com/airbnb/javascript looks nice. It maybe over detailed, but I like especially a small part about jQuery selectors which I already use.

Beside the content of the instruction above I think would be useful to have:

invenio-developers commented 10 years ago

Comment by kneczaj Thursday May 22, 2014 at 12:14 GMT

I think such a document is especially needed for summer students who will come, and write code in their way

invenio-developers commented 10 years ago

Comment by greut Thursday May 22, 2014 at 12:26 GMT

The first point I'd like to enforce regarding JS is no inline JavaScript, no onclick, no href=javascript:

The second would be tests, QUnit is super simple and nice.

Extra pain points: jslint, jsfmt

invenio-developers commented 10 years ago

Comment by kneczaj Thursday May 22, 2014 at 12:29 GMT

Ok, maybe everybody will write here their propositions, and we can discuss/vote them at some meeting later.

invenio-developers commented 10 years ago

Comment by kneczaj Thursday May 22, 2014 at 12:35 GMT

I like naming jQuery selectors with $ sign at the beginning. Also it's important to first save a selector to a variable, and name it somehow, and then use the variable instead of repeating the query to get the selector. It's exactly what is here: https://github.com/airbnb/javascript#jquery Adavantages are:

invenio-developers commented 10 years ago

Comment by drjova Thursday May 22, 2014 at 12:40 GMT

It would be nice if we use a template engine (ex. hogan) to serve html

invenio-developers commented 10 years ago

Comment by greut Thursday May 22, 2014 at 12:40 GMT

@kneczaj And then you start using Angular.js and your world falls apart. Big no-no for $variables.

invenio-developers commented 10 years ago

Comment by kneczaj Thursday May 22, 2014 at 12:45 GMT

jQuery events

I propose not to put big functions directly inside events functions like:

$this.on('click', function(event) { /* here */ });

more than two-three lines of code. When the function is more complicated let's make it externally with proper arguments passed to it (not just jQuery event) and then trigger the function inside the event binding like:

function runQuery(a, b, c) {
}

$this.on('click', function(event) {
   runQuery(event.data.a, event.data.b, event.data.c);
});

Advantages:

There is one issue with it. If you want to run it inside a class, you don't have 'this' inside the event so if runQuery() function above is a class method then such a pattern is useful:

var that = this;
$this.on('click', function(event) {
   that.runQuery(event.data.a, event.data.b, event.data.c);
});
invenio-developers commented 10 years ago

Comment by kneczaj Thursday May 22, 2014 at 12:49 GMT

@greut I haven't met with Angular.js in invenio yet, I also haven't used it by myself, so I am not so familiar with the topic.

@drjova +1 for hogan - btw would be nice to have voting feature here ;)

invenio-developers commented 10 years ago

Comment by greut Thursday May 22, 2014 at 12:52 GMT

Anyway, all these points must be brought during code reviews, arguing in the void has close to no values.

invenio-developers commented 10 years ago

Comment by kneczaj Thursday May 22, 2014 at 12:56 GMT

@greut It's just a random example, but if $this is a jQuery selector made by:

var $this = $(this);

it makes sense for me

invenio-developers commented 10 years ago

Comment by jalavik Thursday May 22, 2014 at 13:51 GMT

Agree with @kneczaj that big inline functions sometimes should be avoided. If the function is big, chances are that parts of it can be split into more functions and re-used elsewhere.

@drjova +1 for hogan and https://github.com/airbnb/javascript looks reasonable at first glance.

Anyway, as we are expanding on the amount of JS being written in Invenio I suggest we land on some JSFramework, as @lnielsen-cern mentioned, to get more stuff and guidelines for free.

invenio-developers commented 10 years ago

Comment by greut Friday May 23, 2014 at 07:28 GMT

I want to quote the co-author of Twitter's Flight, the slide 7 is brilliant.

  • Don't let best practices do your thinking for you
  • Keep it simple and human readable so it's easier to fix

Angus Croll, https://speakerdeck.com/anguscroll/stop-being-perfect

Another good one: https://speakerdeck.com/anguscroll/how-we-learned-to-stop-worrying-and-love-javascript

invenio-developers commented 10 years ago

Comment by giokokos Friday May 23, 2014 at 14:50 GMT

Interesting presentations. Thanks for sharing.

invenio-developers commented 10 years ago

Comment by jirikuncar Wednesday May 28, 2014 at 14:19 GMT

We should seriously discuss usage of AngularJS or similar framework (see also https://github.com/jirikuncar/invenio/pull/207#issuecomment-44408651).

invenio-developers commented 10 years ago

Comment by greut Wednesday May 28, 2014 at 16:33 GMT

And a javascript code loader/dependency like AMD (require.js, ...) or CommonJS (browserify, ...).

For the record, I'll be playing with ReactJS + ... + require.js for my throw-away prototype. I'm new to them and believe in do-ocracy.

invenio-developers commented 10 years ago

Comment by kneczaj Friday Jun 06, 2014 at 16:27 GMT

This is applicable for python as well.

It is always makes code more clear to avoid making nested conditional statements.

To achieve this try to return from a function as fast as possible, and make functions to compute the results of the nested code inside if() statement.

For example instead:

function f(a) {
    if (a) {
        b = a.getSth()
        if (b) {
            c = b.getSth()
            if (c) {
                /* 20 lines of code here */
            }
         }
     }
}

It is much cleaner to write:

function f(a) {
    if (!a)
        return;
   b = a.getSth()
   if (!b)
        return;
   c = b.getSth()
   if (!c)
        return;
    /* 20 lines of code here */
}

This is applicable also to for loops using continue statement instead of return.

Instead:

for (var i=0; i<6; i++) { 
    if (a[i].bool) {
        b = a.getSth()
        if (b) {
            c = b.getSth()
            if (c) {
                /* 20 lines of code here */
            }
         }
     }
}

It is much cleaner to write:

for (var i=0; i<6; i++) {
    if (!a[i].bool)
        continue;
   b = a.getSth()
   if (!b)
        continue;
   c = b.getSth()
   if (!c)
        continue;
    /* 20 lines of code here */
}
kneczaj commented 10 years ago

jQuery Selectors

It is much cleaner and understandable for a third person when jQuery selectors are not strings with 20+ characters.

Try to save intermediate queries in variables, and then make sub-queries on them using find(), parent(), parents(), children() functions.

Avoid storing partial selectors in strings and then combining strings to get the desired one.

This is a good example of what I mean to avoid:

selector_prefix = '#'+options.prefix+options.sep+options.index_suffix+options.sep;
/* later */
var input = root.find(selector_prefix+field);

The above for sure works, but it is not understandable without debugging and very error prone when someone changes the page structure.

jmartinm commented 10 years ago

Regarding the number of spaces (2 or 4), do we have an agreement?

In the last discussion about jinja code style we agreed on 2, although I don't think we agreed on JavaScript. I see some new files like https://github.com/inveniosoftware/invenio/blob/pu/invenio/base/static/js/build.js or https://github.com/inveniosoftware/invenio/blob/pu/invenio/base/static/js/translate.js are using 4 spaces.

It would be good to have a common agreement as we were planning on using the grunt task jsbeautifier and we would like to use the same standard as Invenio - https://github.com/beautify-web/js-beautify#options

It is a matter of taste but it seems like the JS community is going more for 2 spaces http://atroche.org/post/30994290348/javascript-indentation so if you agree we can go for that option.

lnielsen commented 10 years ago

There was no consensus on 2 vs 4 in #1768, except that you shouldn't use 1,3,5,6,7 or 8 spaces, and that you should be consistent through out the file.

greut commented 10 years ago

You can put a .jshintrc file in your module and impose how you expect it to be formatted (https://github.com/greut/cds-demosite/commit/3fc93c40c19e0e276951aba81007b29edc81d1d3). The current state of the JS code is more problematic than its formatting.

drjova commented 10 years ago

Also we may consider chai instead of QUnit, the syntax it's more familiar, it supports language chains and more. Examples from the website:

expect([1,2,3]).to.include(2);
expect('foobar').to.contain('foo');
expect({ foo: 'bar', hello: 'universe' }).to.include.keys('foo');
assert.sameMembers([ 1, 2, 3 ], [ 2, 1, 3 ], 'same members');

QUnit vs Chai

QUnit

QUnit.assert.contains = function( needle, haystack, message ) {
  var actual = haystack.indexOf(needle) > -1;
  QUnit.push(actual, actual, needle, message);
};
QUnit.test("retrieving object keys", function( assert ) {
  var arrayKeys = keys( [1, 2, 3] );
  assert.contains( "3", arrayKeys, "Array keys" );
});

Chai

describe('Test Array', function() {
  it('should not fail', function(done) {
    assert.include([ 1, 2, 3 ], 3, 'array contains value 3');
  });
});
greut commented 10 years ago

Probably, the best and simplest way to do it is to move out any libraries you want to document and test. Then import them with bower as usual in Invenio and test them on Travis via PhantomJS or CasperJS. Just do it®

greut commented 10 years ago

A python friendly library/test runner, Jasmine:

jacquerie commented 9 years ago

It appears that this RFC didn't reach consensus, so no decision was made. Let me try to rekindle the discussion with some organization and some proposals.

The discussion started from code style, then touched code quality, the use of frameworks, testing, and documentation. I think these are very different problems, which require different discussions.

  1. JavaScript code style: IMHO, not as important as the others, but still nice to have. Let me mention JSCS, a configurable tool to enforce code style conventions. Once we decide on a style guide (or perhaps we can lean on the defaults of some project we like), it should make the job much easier.
  2. JavaScript code quality: I think there are two parts to this. First we have the style of JavaScript files: a tool that was already mentioned is JSHint, which will require a discussion on the settings. Then there's the problem of the obtrusiveness of the JavaScript in A LOT of the templates. Judging from the output of ag onclick this will require A LOT of work.
  3. Use of JavaScript frameworks: consensus in #2110 is that this shouldn't be pursued.
  4. Testing JavaScript libraries: RFC #2221 is still open, but, from what I've seen, Jasmine is the de-facto standard.
  5. Documenting JavaScript: RFC #2845 is still open, and this is where it should be discussed.

Related to these, and in particular to the first two, is the question of having some kind of task runner for JavaScript tasks, like Grunt or Gulp. Should I open an RFC for this?

In summary I propose to:

  1. Use this RFC to discuss JSCS's settings
  2. Start a new RFC about JSHint's settings
  3. Start an effort to refactor the templates
  4. Use the existing RFCs to discuss testing libraries and documentation solutions
  5. Start a new RFC about a JavaScript task runner
jacquerie commented 9 years ago

To follow up on yesterday's question, no, JSCS has no "autoformatting" functionality, but it's been actively worked on, so it should be here soon™: https://github.com/jscs-dev/node-jscs/issues/516.

hzoo commented 9 years ago

JSCS just added the --fix option as of 1.12 (on most of the whitespace rules). We need some better docs on it and other things though.

You guys can start with using a preset (I see airbnb was mentioned in the OP) and then nulling out any rules you don't think or adding more that you want. Also feel free to create issues/PRs for new rules/options that could be added in!

jacquerie commented 9 years ago

Awesome. Thanks for correcting me @hzoo!

jacquerie commented 9 years ago

So, let's get back to our discussion. I'll take the following code in Invenio (from https://github.com/inveniosoftware/invenio/blob/ad39db02238330f419bb73d40d708df059d05cf8/invenio/base/static/js/invenio.js#L43-L59) and rewrite it using 3 different presets: AirBnB, Crockford, jQuery.

$('[data-menu="click"]').each(function (index) {
  var that = this,
    $menu = $(this).parent(),
    $li = $menu.parent(),
    message_update = function(obj) {
      $(that).unbind('click');
      $.ajax({
        url: $(that).attr('data-menu-source')
      }).done(function (data) {
        $menu.parent().append(data);
      });
    };
  $(that).on('click', message_update);
  $li.addClass('dropdown');
  $menu.attr('data-toggle', "dropdown");
  $menu.dropdown();
});

1. AirBnB

$('[data-menu="click"]').each(function(index) {
  var _this = this;
  var $menu = $(this).parent();
  var $li = $menu.parent();
  var messageUpdate = function(obj) {
    $(_this).unbind('click');
    $.ajax({
      url: $(_this).attr('data-menu-source')
    }).done(function(data) {
      $menu.parent().append(data);
    });
  };

  $(_this).on('click', messageUpdate);
  $li.addClass('dropdown');
  $menu.attr('data-toggle', 'dropdown');
  $menu.dropdown();
});

2. Crockford

$('[data-menu="click"]').each(function (index) {
    var that = this,
        $menu = $(this).parent(),
        $li = $menu.parent(),
        message_update = function (obj) {
            $(that).unbind('click');
            $.ajax({
                url: $(that).attr('data-menu-source')
            }).done(function (data) {
                $menu.parent().append(data);
            });
        };
    $(that).on('click', message_update);
    $li.addClass('dropdown');
    $menu.attr('data-toggle', "dropdown");
    $menu.dropdown();
});

3. jQuery

$( '[data-menu="click"]' ).each( function( index ) {
  var that = this,
    $menu = $( this ).parent(),
    $li = $menu.parent(),
    messageUpdate = function( obj ) {
      $( that ).unbind( "click" );
      $.ajax( {
        url: $( that ).attr( "data-menu-source" )
      } ).done( function( data ) {
        $menu.parent().append( data );
      } );
    };
  $( that ).on( "click", messageUpdate );
  $li.addClass( "dropdown" );
  $menu.attr( "data-toggle", "dropdown" );
  $menu.dropdown();
} );

Personally, I'd go with number 1.

Here's also an estimate of the effort needed (obtained running jscs -p PRESET invenio/modules/*/static):

  1. AirBnB: 20231 code style errors found.
  2. Crockford: 22815 code style errors found.
  3. jQuery: 33699 code style errors found.
jirikuncar commented 9 years ago

I would like 2. Crockford with 2 space indentation but I'm also fine with 1. AirBnB with possibility to join var definitions.

var foo = 1,
    bar = 'baz',
    fb = function() {
      return 1;
    };
jmartinm commented 9 years ago

1 seems good to me. Agree with Jiri that I would allow join var definitions (as long as they are properly aligned with each other)

bouzlibop commented 9 years ago

I give my vote to 1. AriBnB and as mentioned above - with the possibility to have var definitions joined.

BTW, in regards to proposal 5:

Start a new RFC about a JavaScript task runner

What use cases do you see @jacquerie for js task runner in Invenio in this moment ?

jacquerie commented 9 years ago

As a wrapper for the tooling we introduce in this and several other RFCs, for programmer convenience. For example, grunt jscs might check the code style in all Invenio, using the right paths and excluding files which we don't need to check, without having to remember a long jscs command line.

Another use case I see is pipelining these commands, so that a programmer only needs to type grunt lint and she runs jshint, jscs and jsonlint.

lnielsen commented 9 years ago

It's highly preferable (in my opinion a must) that any linting is integrated with inveniosoftware/kwalitee so that there's only one tool for checking Invenio source code. 1) It's easier for developers (just one tool to run on your commits) 2) Kwalitee is integrated with GitHub so PR's can be checked automatically.

Grunt could potentially be integrated with Kwalitee, so just wanted to emphasise that integration with kwalitee has to be thought of when taking a decision.

bouzlibop commented 9 years ago

I would propose to extend inveniomanage and kwalitee to provide necessary aliases/pipelining/quality checks and avoid adding new tools (grunt).

hzoo commented 9 years ago

You could also use npm to hold scripts if not grunt/gulp. So in package.json (example from jscs itself)

"scripts": {
    "lint": "jshint . && node bin/jscs lib test bin publish"
}

I'l leave this thread now xd

jacquerie commented 9 years ago

:+1:

Ok, I think the consensus is against having a task runner, so I'll drop that proposal.

jalavik commented 9 years ago

@jacquerie We could add it as a suggestion to the invenio kwalitee tool as @lnielsen suggested. Sounds like a nice compromise.