Loopback: Follow lexical convention for `require()`/declaration lines

[pthieu]

Description of feature (or steps to reproduce if bug)

We need to follow a convention for variable declarations. There isn't a documented convention.

Logical:

var express = require('express');
var loopbackExpress = require('./server-app');
var proto = require('./application');
var fs = require('fs');
var ejs = require('ejs');
var path = require('path');
var merge = require('util')._extend;
var assert = require('assert');
var Registry = require('./registry');
var juggler = require('loopback-datasource-juggler');

Above is from https://github.com/strongloop/loopback/blob/806c600fb23e6912603731c5dfbd6ad4ab4deb08/lib/loopback.js -- I'm not sure how this is logical, maybe I've picked a bad file that's not sorted at all.

Lexical:

var Registry = require('./registry');
var assert = require('assert');
var ejs = require('ejs');
var express = require('express');
var fs = require('fs');
var juggler = require('loopback-datasource-juggler');
var loopbackExpress = require('./server-app');
var merge = require('util')._extend;
var path = require('path');
var proto = require('./application');

I'm siding with lexical, because we can add an eslint rule, it requires no thinking, and it's pretty hard to debate b coming before a, whereas Logical will differ depending on the dev.

[pthieu]

/cc @strongloop/loopback-dev @sam-github @bhajian

[superkhau]

I agree with ASCII based lexical ordering.

[bajtos]

+1 from me.

Just keep in mind that lexical ordering is not feasible in certain situations, e.g.

var util = require('util');
var extend = util._extend;
var inherits = util.inherits;

If we agree to introduce lexical ordering, then the following document should be updated: https://github.com/strongloop/loopback.io/blob/gh-pages/pages/en/contrib/style-guide.md

[Amir-61]

👍 for Lexical ordering and documenting in our KB.

[superkhau]

Just keep in mind that lexical ordering is not feasible in certain situations, e.g.

Regarding this -- I think we should space out the actual operations that use the requires:

var util = require('util');

varr extend = util._extend;
var inherits...

That way, we can treat it as actual var declarations (where order matters) instead of lumping those together with imports (and now doesn't stop the lexical ordering because they are not part of the import list).

[pthieu]

Just keep in mind that lexical ordering is not feasible in certain situations, e.g.

I'm in agreement, Seems like a grey area here though. What happens if you only need .extend?

var a = require('a');
var extend = require('util').extend
var z = require('z');

I'm believe the above is simpler, but not sure if SL wants:

var a = require('a');
var z = require('z');

var extend = require('util').extend

[superkhau]

var util = require('util');

var extend = util.extend;

OR

var extend = require('util').extend;

But I hear you, it's definitely subjective. If you're going to enforce conventions, those edge cases should be clear so there are no debates. We should stop inlining imports too -- this one is also subjective too depending on whether there is a perf impact.

[pthieu]

So basically, we're instating the rule to always require the root module and assign any methods inside afterwards in a separate block. I'm ok with this as long as it's defined somewhere, which I guess I'll do when we have all the rules set.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.