New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/eslint #1416

Merged
merged 8 commits into from May 18, 2016

Conversation

Projects
None yet
6 participants
@rhys-vdw
Collaborator

rhys-vdw commented May 16, 2016

Okay... Quite a big diff here.

This is something I've been meaning to do for a while. The main goal of this change is to get PRs reviewed and merged quicker, with fewer errors and higher code quality. All linter warnings are errors in CI, so we don't even need to look at PR's until they're conforming to the rules given in .eslintrc.js.

I did most of the conversion with esnext, then manually corrected the rest. Our test coverage is at 86%, so hopefully I didn't introduce any regressions. Obviously I'm not expecting anybody to do a line-by-line review here. 馃槈

My main apprehension with this is that it's likely to cause problems with any existing PR. Hopefully most of this pain can be ameliorated with esnext, but it's certainly not a magic bullet. On the other hand I really want to get this merged quickly, since it touches literally everything. Awkward either way, but I think it's for the best.

"plaintest": "mocha --check-leaks -t 10000 -b -R spec test/index.js && npm run tape",
"prepublish": "npm run build",
"tape": "node test/tape/index.js | tap-spec",
"test": "npm run babel && istanbul --config=test/.istanbul.yml cover node_modules/mocha/bin/_mocha -- --check-leaks -t 5000 -b -R spec test/index.js && npm run tape"

This comment has been minimized.

@wolfgang42

wolfgang42 May 16, 2016

Contributor

It looks like test/ is no longer linted, and linting isn't run as part of npm run test anymore. Is there a reason for this, or did I miss something?

This comment has been minimized.

@rhys-vdw

rhys-vdw May 16, 2016

Collaborator

That's right. The new linting rules disallow var, but the test framework is all es5. Perhaps I discarded jshint prematurely. Ultimately I'd like to get the tests compiled also.

This comment has been minimized.

@villelahdenvuo

villelahdenvuo May 20, 2016

Contributor

I have created another .eslintrc file for my test folder to allow mocha and other global helpers for example. Edit: Because eslint extends the config you don't have to rewrite the whole file.

var defaultEnv = 'development';
var config = require(env.configPath);
let environment = commander.env || process.env.NODE_ENV;
const defaultEnv = 'development';

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

misaligned = (btw. why do we align those at all?)

dispose = poolConfig.destroy
}
poolDefaults(poolConfig) {
const client = this

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

hmm... this also silently removes a deprecated feature - intentional?

This comment has been minimized.

@rhys-vdw

rhys-vdw May 18, 2016

Collaborator

@jurko-gospodnetic Yeah, so I discovered this because ESLint was complaining about dispose never being used. I traced the history of this feature back and it seems very old (the history was disrupted by the introduction of Babel, I think over a year ago). I think it's safe to remove.

Who knows what dispose was meant to do. Whatever it was it wasn't doing it.

var helpers = require('../../helpers')
import inherits from 'inherits';
import Debug from 'debug';
import {assign} from 'lodash'

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

in all new places in code I see, we have spaces around the words inside braces, i.e. according to that style this would be written as import { assign } from 'lodash'

var inherits = require('inherits')
var Formatter = require('../../formatter')
import inherits from 'inherits';
import Formatter from '../../formatter';
import {assign} from 'lodash'

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

as before - the whitespace around assign seems to be missing here when compared to the new code

if (!sql) return resolver()
if (obj.options) sql = assign({sql: sql}, obj.options).sql
var req = (connection.tx_ || connection).request();
if (obj.options) ({ sql } = assign({sql}, obj.options))

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

why the extra parentheses here?

This comment has been minimized.

@rhys-vdw

rhys-vdw May 18, 2016

Collaborator

Beats me. I think that was autogenerated.

var response = obj.response
var method = obj.method
let { response } = obj
const { method } = obj

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

misaligned equal sign

sql += ') ' + returningSql + 'values ('
var i = -1
sql += `(${this.formatter.columnize(insertData.columns)}`
sql += `) ${returningSql}values (`

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

might as well merge these two lines, since we're using template string literals

const where = this.where();
const order = this.order();
const top = this.top();
const { returning } = this.single;

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

misaligned =

import inherits from 'inherits';
import Promise from '../../promise';
import Transaction from '../../transaction';
const debug = require('debug')('knex:tx')

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

strange = alignment

const { response } = obj
const { method } = obj
const rows = response[0]
const fields = response[1]

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

strange = alignment

// see: http://stackoverflow.com/questions/255517/mysql-offset-infinite-rows
const limit = (this.single.offset && noLimit)
? '18446744073709551615'
: this.formatter.parameter(this.single.limit)

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

missing ;?

const { response } = obj
const { method } = obj
const rows = response[0]
const fields = response[1]

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

misaligned =

var helpers = require('../../helpers')
import Transaction from '../../transaction';
import inherits from 'inherits';
const debug = require('debug')('knex:tx')

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

strange = alignment

var method = obj.method;
processResponse(obj, runner) {
let { response } = obj;
const { method } = obj;

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

strange = alignment

var returning = this.single.returning;
insert() {
let insertValues = this.single.insert || []
let { returning } = this.single;

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

strange = alignment

import inherits from 'inherits';
import Promise from '../../promise';
import Transaction from '../../transaction';
const debugTx = require('debug')('knex:tx')

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

strange = alignment

update() {
const updateData = this._prepUpdate(this.single.update);
const wheres = this.where();
const { returning } = this.single;

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

strange = alignment

var response = obj.response;
processResponse(obj, runner) {
const ctx = obj.context;
let { response } = obj;

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

strange = alignment

@@ -16,7 +16,7 @@ inherits(Transaction_WebSQL, EventEmitter)
function makeClient(trx, client) {
var trxClient = Object.create(client.constructor.prototype)
const trxClient = Object.create(client.constructor.prototype)

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

strange = alignment

const {tableName, disableTransactions} = this.config
const directory = this._absoluteConfigDir()
let current = Promise.bind({failed: false, failedOn: 0});
const log = [];

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

strange = alignment

each(migrations, (migration) => {
var name = migration;
const name = migration;

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

strange = alignment

if (typeof insertData === 'string') {
sql += insertData;
} else {
if (insertData.columns.length) {
sql += '(' + this.formatter.columnize(insertData.columns)
sql += `(${this.formatter.columnize(insertData.columns)}`
sql += ') values ('

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

might as well merge these two lines since we're using template string literals

var updateData = this._prepUpdate(this.single.update);
var wheres = this.where();
return 'update ' + tableName +
const { tableName } = this;

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

strange = alignment

var i = -1
const vals = []
const sorted = Object.keys(data).sort()
let i = -1

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

strange = alignment

return this.on.apply(this, arguments);
},
// Adds an "or on" clause to the current join object.
orOn: function(first, operator, second) {
orOn(first, operator, second) {
/*jshint unused: false*/

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

are the jshint comments still needed?

src/raw.js Outdated
const values = raw.bindings
const { client } = raw
let index = 0;
let bindings = []

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

real strange = alignment

src/raw.js Outdated
var sql = raw.sql, bindings = []
const values = raw.bindings
const { client } = raw
let { sql } = raw, bindings = []

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

strange = alignment

createdAt.notNullable().defaultTo(now);
updatedAt.notNullable().defaultTo(now);
}
return;
}
var builder = this.client.columnBuilder(this, type, args);
const builder = this.client.columnBuilder(this, type, args);

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

strange = alignment

const { knex } = this;
const seedDirectory = this._absoluteConfigDir();
let current = Promise.bind({failed: false, failedOn: 0});
const log = [];

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

strange = alignment

each(seeds, function(seed) {
var name = path.join(seedDirectory, seed);
const name = path.join(seedDirectory, seed);

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

strange = alignment

var makeKnex = require('./util/make-knex')
var debug = require('debug')('knex:tx')
import makeKnex from './util/make-knex';
const debug = require('debug')('knex:tx')

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

strange = alignment

@@ -197,7 +187,7 @@ function makeTransactor(trx, connection, trxClient) {
// connection and does not release back into the pool.
function makeTxClient(trx, client, connection) {
var trxClient = Object.create(client.constructor.prototype)
const trxClient = Object.create(client.constructor.prototype)

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

strange = alignment

export default () => require('bluebird')

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

add an end-of-line character at the end of this file?

This comment has been minimized.

@rhys-vdw

rhys-vdw May 18, 2016

Collaborator

This is always inconsistent between files due to different editors. I'm not sure what the preference is here.

client.makeKnex = function(client) {
return makeKnex(client)
}
client.makeKnex = client => makeKnex(client)

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

why not just client.makeKnex = makeKnex?

@@ -3,8 +3,8 @@ import url from 'url'
import {parse as parsePG} from 'pg-connection-string'
export default function parseConnectionString(str) {
var parsed = url.parse(str)
var protocol = parsed.protocol
const parsed = url.parse(str)

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 17, 2016

Collaborator

strange = alignment

@tgriesser

This comment has been minimized.

Owner

tgriesser commented May 17, 2016

So actually, could we hold off on merging this for the time being. As I mentioned to rhys, I'll be back early next week (probably moreso in gitter) to chime in with some plans for esnext and beyond, and to share some of what I've been working on around making things modular and better tested with http://lernajs.io. While I also want to get to eslint asap, there's a lot of internal design decisions I'd like to re-evaluate and I'm not sure that touching every line of the codebase right now will help with that :)

One thing I'd like to do is get away from using es6 imports until they are a standard in node land. Ideally we'd be able to write code that works fully against node 6, can be transpiled minimally down to node 4 and then will look pretty ugly but will work against anything earlier than that, with something like:

if (featureTest6plus) {
  module.exports = require('./src')
} else if (featureTest4) {
  module.exports = require('./lib')
} else {
  module.exports = require('./es5')
}
@jurko-gospodnetic

This comment has been minimized.

Collaborator

jurko-gospodnetic commented May 17, 2016

@tgriesser - FWIW, making a change like this is always going to be painful unless you can get your project to a 'clean slate' state, meaning - no one working on anything at the moment. 馃槅

You can tear the band-aid off quick (i.e. merge) or slow (i.e. merge in parts), but it's going to smart either way. 馃槅

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented May 17, 2016

So actually, could we hold off on merging this for the time being.

Sure thing.

However that runs the risk of this being very difficult to merge later. I spent a bit of time on this, so if there was another way I'd prefer to take that route.

One thing I'd like to do is get away from using es6 imports until they are a standard in node land.

import/export semantics have the advantage of being statically analysed by eslint-plugin-import. AFAIK there is no possible equivalent for CommonJS semantics. There is no reason why we can't use the two interchangeably (aside from no native Node support).

Ideally we'd be able to write code that works fully against node 6, can be transpiled minimally down to node 4 and then will look pretty ugly but will work against anything earlier than that

I actually explored this approach in the past when configuring Babel for another project, taking advice from team members on their Slack channel. Can't recall the reasoning but apparently this pattern is discouraged.

Are there advantages to this, or would it just be a stepping stone to dropping a reliance on Babel?

With regards to dealing with this PR, how would you feel about me removing a few Babel transformers and reducing ESLint compatibility so that it's fully supported by Node 6 right now, @tgriesser?

@tgriesser

This comment has been minimized.

Owner

tgriesser commented May 17, 2016

However that runs the risk of this being very difficult to merge later.

Actually, feel free to merge - I was doing a lot of this stuff myself when splitting out code so it's probably simpler to work from a full (cleaner) es2015 base code

Can't recall the reasoning but apparently this pattern is discouraged.

Interested to hear more about why this is discouraged

Are there advantages to this, or would it just be a stepping stone to dropping a reliance on Babel?

Yeah, mainly a stepping stone to a reliance on babel, cleaner stack traces & source.

import/export semantics have the advantage of being statically analysed by eslint-plugin-import. AFAIK there is no possible equivalent for CommonJS semantics. There is no reason why we can't use the two interchangeably (aside from no native Node support).

Yeah that's mainly it, the native node support. I'm not completely set against them, the static analysis is definitely a big benefit. I think I just wanted to get away from transpiling as much as we can as we get pretty close to full es2015 support in node, and then we can add in new features as they land in a node LTS release.

rhys-vdw added some commits May 16, 2016

Remove duplicate spaces in assignments.
These were previously horizontally aligned. The esnext conversion disrupted them all.
Make spaces in import braces consistent
`import {module}` -> `import { module }`
Merge branch 'master' into feature/eslint
# Conflicts:
#	src/client.js
#	src/dialects/mssql/transaction.js
#	src/dialects/oracle/transaction.js
#	src/runner.js
#	src/transaction.js

@rhys-vdw rhys-vdw merged commit d40e578 into master May 18, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented May 18, 2016

Actually, feel free to merge - I was doing a lot of this stuff myself when splitting out code so it's probably simpler to work from a full (cleaner) es2015 base code

Thanks. I hope it wont be too painful. It may be that we have to totally lock down PRs entirely for some period when the transition occurs.

Interested to hear more about why this is discouraged

We currently offer source maps, which I believe should show correct traces if client code is using source-map-support.

The only argument against it I can think of (besides unnecessary complexity) would be that exported file paths are based off main in package.json. So if you have three source files (and you wish to expose submodules to require) then, to ensure portability, users will have to select the submodule with the most portable code:

// Fails to parse older versions of Node!
const QueryBuilder = require('knex/lib/QueryBuilder');

Perhaps that wont matter if using Lerna, but it still seems strange.

How about publishing multiple packages on release, one that is transpiled and one other?

@jurko-gospodnetic jurko-gospodnetic deleted the feature/eslint branch May 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment