New: eslint-visitor-keys #1
Conversation
|
Please don't merge this PR in yet. JSF prefers Apache license to MIT. So while we still don't have any code in the repository, it would be easier to change the license. |
There was a problem hiding this comment.
This generally looks good to me, I just have a few questions.
Also, I'm not sure how I can setup scripts to release this package.
It might only be necessary to add an eslint-release script (like this one in eslint-scope). It looks like Makefile.js in eslint-scope doesn't have anything related to the release.
README.md
Outdated
| For example: | ||
|
|
||
| ``` | ||
| console.log(evk.getKeys()) // → ["left", "right"] |
There was a problem hiding this comment.
Should this call have an argument?
There was a problem hiding this comment.
Oh, yes! This is a copy-paste mistake.
| const KEYS = require("./visitor-keys.json"); | ||
|
|
||
| // Types. | ||
| const NODE_TYPES = Object.freeze(Object.keys(KEYS)); |
There was a problem hiding this comment.
Does NODE_TYPES need to be frozen? It seems like it's not exported.
(I suppose it's not a problem to freeze it either way.)
There was a problem hiding this comment.
Because of my liking. It clarifies that it's a constant.
|
|
||
| it("should not have duplicate", () => { | ||
| assert(unionKeys.Program.filter(key => key === "body").length === 1); | ||
| }); |
There was a problem hiding this comment.
Should we add tests for preserving the order of keys?
evk.unionWith({ CallExpression: ["callee"] }).CallExpression;
// ["callee", "arguments"]?
// ["arguments", "callee"]?- fix the doc of `getKeys` - clarify the order of keys for `unionWith`
|
I update this PR to follow the review. I rethought about the order of keys for |
|
Thanks for driving this, @mysticatea! I'll use this PR commit in my babel-eslint PR and see where we're at |
|
@mysticatea Your freezing of the keys was very useful in flagging up a remaining case of mutation happening within babel-eslint. The contents of this function are unchanged in my PR, but the file it is now in is different: // set ArrayPattern/ObjectPattern visitor keys back to their original. otherwise
// eslint-scope will traverse into them and include the identifiers within as declarations
extendedVisitorKeys.ObjectPattern = ["properties"];
extendedVisitorKeys.ArrayPattern = ["elements"];
// Execute default Referencer logic
super.visitFunction(node);
// set them back to normal...
extendedVisitorKeys.ObjectPattern = t.VISITOR_KEYS.ObjectPattern;
extendedVisitorKeys.ArrayPattern = t.VISITOR_KEYS.ArrayPattern;What do you think about this? Should we try and expose an API in this new project for temporarily overriding keys adhoc like this? Or is there a better way? |
|
@JamesHenry Does it behave differently about patterns between inside of functions and outside of functions? I think const {a} = obj // original visitor keys.
function foo() {
const {a} = obj // overwritten visitor keys.
} |
|
I don't know, I'm afraid. That PR is my first interaction with babel-eslint, and (eslint-scope's Referencer in general), and as I say this specific bit is not my code... |
|
I will have much more time next week for open-source, I am a bit swamped right now |
|
Thanks. I think that we don't support temporarily overriding for keys in this repo. Traversing is recursive, so it causes unintentional behaviors in general. |
|
May I merge this? |
|
👍 |
This PR is the first implementation of
eslint-visitor-keys.Please review for direction.
Also, I'm not sure how I can setup scripts to release this package.
Please help me.