Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to Babel parser & Change JSHint to ESLint & Drop Support for Node 6 #61

Merged
merged 2 commits into from
Jul 1, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
/tests/fixtures/
67 changes: 67 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
module.exports = {
root: true,
parserOptions: {
ecmaVersion: 2018,
},
plugins: ['mocha', 'node'],
extends: ['eslint:recommended', 'plugin:node/recommended', 'prettier'],
env: {
browser: false,
node: true,
es6: true,
},
globals: {},
rules: {
/*** For compatibility with existing source */
'no-var': 0,
'no-undef': 0,
'strict': 0,
'object-shorthand': 0,
'no-lonely-if': 0,

/*** Possible Errors ***/

'no-console': 0,
'no-template-curly-in-string': 2,
'no-unsafe-negation': 2,

/*** Best Practices ***/

curly: 2,
eqeqeq: 2,
'guard-for-in': 0,
'no-caller': 2,
'no-eq-null': 2,
'no-eval': 2,
'no-new': 0,
'no-unused-expressions': [
2,
{
allowShortCircuit: true,
allowTernary: true,
},
],
'wrap-iife': 0,
yoda: 2,

/*** Variables ***/

'no-unused-vars': 2,
'no-use-before-define': [2, 'nofunc'],

/*** Stylistic Issues ***/

camelcase: 2,
'new-cap': [2, { properties: false }],
'no-array-constructor': 2,
'no-bitwise': 2,
'no-plusplus': 0,
'no-unneeded-ternary': 2,

/*** ECMAScript 6 ***/

'no-useless-computed-key': 2,
'prefer-template': 2,
'symbol-description': 2,
},
};
5 changes: 0 additions & 5 deletions .jshintrc

This file was deleted.

7 changes: 7 additions & 0 deletions .prettierrc.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
'use strict';

module.exports = {
singleQuote: true,
trailingComma: 'es5',
printWidth: 120,
};
1 change: 0 additions & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
language: node_js
node_js:
- '6'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should probably be considered a breaking change and will require a new major version release

- '8'
- '10'

Expand Down
3 changes: 2 additions & 1 deletion lib/ember-router-generator.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
module.exports = EmberRouterGenerator;

var recast = require('recast');
var parser = require('recast/parsers/babel');

var findFunctionExpression = require('./helpers/find-function-expression');
var findRoutes = require('./helpers/find-routes');
Expand All @@ -21,7 +22,7 @@ EmberRouterGenerator.prototype.clone = function() {
};

EmberRouterGenerator.prototype._ast = function() {
return (this.ast = this.ast || recast.parse(this.source));
return (this.ast = this.ast || recast.parse(this.source, { parser }));
};

EmberRouterGenerator.prototype._walk = function() {
Expand Down
12 changes: 9 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,20 @@
"url": "https://github.com/ember-cli/ember-router-generator.git"
},
"dependencies": {
"@babel/parser": "^7.4.5",
"@babel/traverse": "^7.4.5",
"recast": "^0.18.1"
},
"devDependencies": {
"assertion-error": "^1.1.0",
"chai": "^4.2.0",
"esprima": "^2.7.2",
"eslint": "^6.0.1",
"eslint-config-prettier": "^6.0.0",
"eslint-plugin-mocha": "^5.3.0",
"eslint-plugin-node": "^9.1.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it's not too much work, could you extract the JSHint->ESLint change to a separate PR? that would make the parser changes here significantly easier to review 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to separate it completely but it was non-trivial because some of the code in this PR was incompatible with the old version of jshint. I tried to make the code changes as minimal as possible. If it makes it easier, almost all parser-related changes are in the code, not configuration files (other than the new Babel deps in package.json).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds like an ordering problem to me. if switching to ESLint is necessary for the code in this PR then why not switch to ESLint first, before modifying any code? I assume the ESLint change does not depend on the new parser code in this PR.

"mocha": "^6.1.4",
"mocha-jshint": "2.3.1"
"mocha-eslint": "^5.0.0",
"prettier": "^1.18.2"
},
"author": "Adolfo Builes",
"license": "MIT",
Expand All @@ -35,6 +41,6 @@
},
"homepage": "https://github.com/ember-cli/ember-router-generator",
"engines": {
"node": "6.* || 8.* || >= 10.*"
"node": "8.* || 10.* || >= 12"
}
}
8 changes: 8 additions & 0 deletions tests/fixtures/class-syntax-foos-route.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
class AppRouter extends Router {
@someDecorator location = config.locationType;
rootURL = config.rootURL;
}

Router.map(function() {
this.route('foos');
});
7 changes: 7 additions & 0 deletions tests/fixtures/class-syntax.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class AppRouter extends Router {
@someDecorator location = config.locationType;
rootURL = config.rootURL;
}

Router.map(function() {
});
15 changes: 10 additions & 5 deletions tests/generator-test.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
var EmberRouterGenerator = require('../index.js');
var assert = require('assert');
var fs = require('fs');
var astEquality = require('./helpers/esprima-ast-equality');
var recast = require('recast');
var expect = require('chai').expect;

var astEquality = require('./helpers/ast-equality');

describe('Adding routes', function() {
it('adds routes', function() {
Expand All @@ -16,6 +12,15 @@ describe('Adding routes', function() {
astEquality(newRoutes.code(), fs.readFileSync('./tests/fixtures/foos-route.js'));
});

it('handles files with class syntax', function() {
var source = fs.readFileSync('./tests/fixtures/class-syntax.js');

var routes = new EmberRouterGenerator(source);
var newRoutes = routes.add('foos');

astEquality(newRoutes.code(), fs.readFileSync('./tests/fixtures/class-syntax-foos-route.js'));
});

it('leaves untouched existing routes', function() {
var source = fs.readFileSync('./tests/fixtures/foos-route.js');

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,16 @@ EqualityError.prototype = Object.create(Error.prototype);
EqualityError.prototype.name = 'EqualityError';
EqualityError.prototype.constructor = EqualityError;

var assert = require('assert');
var esprima = require('esprima');
var recast = require('recast');
var parser = require('recast/parsers/babel');
var traverse = require('@babel/traverse').default;

module.exports = function(actual, expected, message) {
var parsedActual = esprima.parse(actual);
var parsedExpected = esprima.parse(expected);
var parsedActual = recast.parse(actual.toString(), { parser });
var parsedExpected = recast.parse(expected.toString(), { parser });

// Removes source-specific metadata from AST nodes
stripSourceInfo(parsedActual, parsedExpected);

var seemEqual = JSON.stringify(parsedActual) === JSON.stringify(parsedExpected);

Expand All @@ -27,3 +30,15 @@ module.exports = function(actual, expected, message) {
);
}
};

function stripSourceInfo(ast1, ast2) {
const stripNode = node => {
delete node.tokens;
delete node.loc;
delete node.start;
delete node.end;
}

traverse.cheap(ast1, stripNode);
traverse.cheap(ast2, stripNode);
}
2 changes: 1 addition & 1 deletion tests/jshint-test.js
Original file line number Diff line number Diff line change
@@ -1 +1 @@
require('mocha-jshint')();
require('mocha-eslint')(['lib', 'index.js', 'tests/*.js', 'tests/helpers']);
Loading