Breaking: align babel-eslint-parser & espree #11137
Conversation
kaicataldo
force-pushed
the
use-jest-babel-eslint
branch
from
6b15391
to
2d77d45
Compare
| delete node.extra; | ||
| } | ||
|
|
||
| if (node.loc && Object.hasOwnProperty.call(node.loc, "identifierName")) { |
There was a problem hiding this comment.
Seems unlikely ESLint would need this, but please let me know if I'm wrong. I also don't see this referenced anywhere in eslint-plugin-flowtype.
| const node = path.node; | ||
| const { node } = path; | ||
|
|
||
| if (Object.hasOwnProperty.call(node, "extra")) { |
There was a problem hiding this comment.
Seems unlikely ESLint would need this, but please let me know if I'm wrong. I also don't see this referenced anywhere in eslint-plugin-flowtype.
There was a problem hiding this comment.
AFAIK node.extra is only used internally in babel-parser. It is a collection of per-node parser features, parenthesized, trailingComma, etc. So we can safely remove node.extra.
I think we can even remove the hasOwnProperty guard then.
| ast.body = ast.program.body; | ||
| delete ast.program; | ||
| delete ast.errors; |
There was a problem hiding this comment.
Extra property that ESLint doesn't use (though we are planning to implement a top-level recoverableErrors property - you can find more info here).
kaicataldo
force-pushed
the
use-jest-babel-eslint
branch
2 times, most recently
from
990e31f
to
0ca7c3b
Compare
kaicataldo
changed the title
| @@ -31,7 +31,6 @@ | |||
| "@babel/core": "^7.2.0", | |||
| "@babel/eslint-shared-fixtures": "*", | |||
| "eslint": "^6.0.1", | |||
| "espree": "^6.0.0", | |||
There was a problem hiding this comment.
Thoughts on this? Seems like we should ensure we're using the version of Espree that's included with the version of ESLint we're testing against.
There was a problem hiding this comment.
i think it should be a combo peer dep and dep, that way npm ls will fail (and npm install in npm 7+) when the peer dep isn't met.
There was a problem hiding this comment.
@nicolo-ribaudo @JLHwung @existentialism Thoughts?
There was a problem hiding this comment.
Now that we use require.resolve to get ESLint's dep, this shouldn't be needed anymore.
| @@ -107,12 +107,7 @@ function convertProgramNode(ast) { | |||
| if (ast.comments.length) { | |||
| const lastComment = ast.comments[ast.comments.length - 1]; | |||
|
|
|||
| if (!ast.tokens.length) { | |||
| // if no tokens, the program starts at the end of the last comment | |||
There was a problem hiding this comment.
This behavior doesn't match Espree's (it probably did at one point and has since changed).
| @@ -306,13 +306,11 @@ describe("Babel and Espree", () => { | |||
| expect(babylonAST.tokens[3].value).toEqual("#"); | |||
| }); | |||
|
|
|||
| // eslint-disable-next-line jest/no-disabled-tests | |||
| it.skip("empty program with line comment", () => { | |||
| it("empty program with line comment", () => { | |||
There was a problem hiding this comment.
This is now aligned! 🎉
kaicataldo
added
PR: Breaking Change 💥
nicolo-ribaudo
dismissed
their stale review
I didn't meant to approve, I haven't reviewed yet 😅
| const ALLOWED_PROPERTIES = [ | ||
| "importKind", | ||
| "exportKind", | ||
| "variance", |
There was a problem hiding this comment.
This is used in eslint-plugin-flowtype.
| "importKind", | ||
| "exportKind", | ||
| "variance", | ||
| "typeArguments", |
There was a problem hiding this comment.
This is not used in eslint-plugin-flowtype. Should we remove this?
There was a problem hiding this comment.
I think that we should keep it: if espree doesn't support flow, we don't need to align in this case.
| configFile: require.resolve( | ||
| "@babel/eslint-shared-fixtures/config/babel.config.js", | ||
| ), | ||
| }; | ||
| const ALLOWED_PROPERTIES = [ | ||
| "importKind", |
There was a problem hiding this comment.
I'm assuming we want to leave importKind and exportKind since they might be useful for Flow-related ESLint rules. exportKind is already used in eslint-plugin-flowtype, so it seems like we should include both properties.
There was a problem hiding this comment.
eslint-plugin-import definitely uses these.
There was a problem hiding this comment.
We might want to add tests for third-party ESLint plugins in the e2e Circle CI workflow.
kaicataldo
force-pushed
the
use-jest-babel-eslint
branch
from
0c8119b
to
a9dc043
Compare
|
@kaicataldo The failing tests are related to the changes in this PR. |
|
Yikes, had a bad rebase or something and lost a bunch of the changes 🤦♂. Fixing up now. Edit: I see what I did now. I had an outdated branch on another machine and rebased and force pushed. Oof. Edit 2: TIL about |
kaicataldo
force-pushed
the
use-jest-babel-eslint
branch
from
59b4950
to
e77e6af
Compare
kaicataldo
force-pushed
the
use-jest-babel-eslint
branch
from
e77e6af
to
0c8119b
Compare
kaicataldo
force-pushed
the
use-jest-babel-eslint
branch
from
0c8119b
to
3b84c20
Compare
|
|
| const node = path.node; | ||
|
|
||
| // private var to track original node type | ||
| node._babelType = node.type; |
There was a problem hiding this comment.
I was wondering why we had this in the first place, but a quick search on GitHub (search) shows that it's only used in snapshot tests output: it can be safely removed.
github-actions
bot
added
the
outdated
As I was switching our test suite to use Jest, I noticed that we're not checking for extraneous properties on nodes. I'm still looking into which properties might be useful to keep (even if they're not defined by ESTree), but this way we can be explicit about what extra properties we decide to keep.
This is a breaking change because it's removing some properties from nodes in the
@babel/eslint-parserAST.