Switch to Babel parser & Change JSHint to ESLint & Drop Support for Node 6 #61
Conversation
tests/fixtures/class-syntax.js
Outdated
| @@ -0,0 +1,7 @@ | |||
| class AppRouter extends Router { | |||
| @tracked location = config.locationType; | |||
There was a problem hiding this comment.
Seems good to confirm decorators don’t blow up parsing (which I assume is the point), but just wanted to confirm that we are on the same page: it’s super unlikely that we’d allow this specific field as tracked, right?
There was a problem hiding this comment.
Correct, making this tracked would have no effect. I'd be happy to change the test if you can recommend an example that would be less confusing.
There was a problem hiding this comment.
Maybe just @someDecorator makes it clear that we're testing for abstract decorator support, not a specific expected feature?
There was a problem hiding this comment.
Ya, probably but its fine either way to me.
| @@ -1,6 +1,5 @@ | |||
| language: node_js | |||
| node_js: | |||
| - '6' | |||
There was a problem hiding this comment.
this should probably be considered a breaking change and will require a new major version release
| "eslint": "^6.0.1", | ||
| "eslint-config-prettier": "^6.0.0", | ||
| "eslint-plugin-mocha": "^5.3.0", | ||
| "eslint-plugin-node": "^9.1.0", |
There was a problem hiding this comment.
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 🙏
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
This commit changes the parser from esprima (the default parser that comes with recast) to @babel/parser. It uses the pre-configured Babel parser shipped with recast (recast/parsers/babel) that ensures the AST returned by Babel is compatible with recast. The goal of this change is to support newer JavaScript syntax that is not yet supported by Esprima, so this commit also includes a new test where the input source includes class syntax, class fields, and a decorator. I also replaced jshint with eslint, based on the current ember-cli configuration, but adjusted the rules to be looser to avoid noise in the diff. We can tighten up the rules and update the source in a separate change. This also adds a prettier config pulled from ember-cli, but does not enforce the rules in the test suite at this time (again, to avoid a noisy diff making it hard to understand the Babel/Esprima parser change).
|
Updated |
Turbo87
changed the title
|
We should confirm that |
This PR changes the parser from Esprima (the default) to Babel. It uses a pre-configured parser shipped with recast (
recast/parsers/babel) so that the AST returned by Babel is compatible.The goal of this change is to support newer JavaScript syntax that is not yet supported by Esprima, so this PR also includes a new test where the input source includes a class, class fields, and a decorator.
I also replaced jshint with eslint, based on the current ember-cli configuration, but adjusted the rules to be looser to avoid noise in the diff. We can tighten up the rules and update the source in a separate change.
This also adds a prettier config pulled from ember-cli, but does not enforce the rules in the test suite at this time (again, to avoid a noisy diff making it hard to understand the Babel/Esprima parser change).