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

Add support for custom visitor keys. #112

Merged
merged 4 commits into from Feb 2, 2021

Conversation

ota-meshi
Copy link
Contributor

This PR supports custom visitor keys by adding options to query, traverse, matches and match.
Also, I changed to traverse using Object.keys when no option is specified. (Same as when using 'iteration' for estraverse.Visitor.fallback.)

closes #111

You can set the following keys for the options:

  • visitorKeys ... if specified, extend the properties of the nodes that traverse the node.
  • fallback ... if specified, control the properties of traversing nodes when encountering unknown nodes. If not specified, use Object.keys.

This option allows you to use queries such as ':nth-child', ':first-child', ' ~ ' and ' + ' in JSX and TypeScript ASTs.

@brettz9
Copy link
Contributor

brettz9 commented Feb 2, 2021

This strikes me as a pretty useful issue to have resolved, so one is not locked in. Could we get a review perhaps, @michaelficarra ?

This is apparently currently blocking eslint/eslint#13639 as per eslint/eslint#13639 (comment) . And as such, this appears to be coming up for us in eslint-plugin-jsdoc when users supply their own contexts to indicate precisely where they want jsdoc blocks attached in TS (appears to impact :has() also due to it using a hard-coded-keys traverse).

esquery.js Outdated
candidates = options.fallback(node);
} else {
// 'iteration' fallback
candidates = Object.keys(node);
Copy link
Member

Choose a reason for hiding this comment

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

Should we omit type from here?

esquery.js Outdated
@@ -224,19 +237,55 @@ function matches(node, selector, ancestry) {
throw new Error(`Unknown selector type: ${selector.type}`);
}

/**
* Get visitor keys of a given node.
* @param {external:AST} node node The AST node to get keys.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param {external:AST} node node The AST node to get keys.
* @param {external:AST} node The AST node to get keys.

esquery.js Outdated
Comment on lines 248 to 262
let candidates;
if (options && options.visitorKeys && options.visitorKeys[nodeType]) {
candidates = options.visitorKeys[nodeType];
} else {
candidates = estraverse.VisitorKeys[nodeType];
}
if(!candidates) {
if (options && typeof options.fallback === 'function') {
candidates = options.fallback(node);
} else {
// 'iteration' fallback
candidates = Object.keys(node);
}
}
return candidates;
Copy link
Member

Choose a reason for hiding this comment

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

I would structure this function as early returns instead of a bunch of assignments.

Suggested change
let candidates;
if (options && options.visitorKeys && options.visitorKeys[nodeType]) {
candidates = options.visitorKeys[nodeType];
} else {
candidates = estraverse.VisitorKeys[nodeType];
}
if(!candidates) {
if (options && typeof options.fallback === 'function') {
candidates = options.fallback(node);
} else {
// 'iteration' fallback
candidates = Object.keys(node);
}
}
return candidates;
if (options && options.visitorKeys && options.visitorKeys[nodeType]) {
return options.visitorKeys[nodeType];
}
if (estraverse.VisitorKeys[nodeType]) {
return estraverse.VisitorKeys[nodeType];
}
if (options && typeof options.fallback === 'function') {
return options.fallback(node);
}
// 'iteration' fallback
return Object.keys(node);

Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM.

@ota-meshi
Copy link
Contributor Author

@michaelficarra Thank you for reviewing!
I have made the changes you requested.

But travis-ci doesn't seem to work. We may need to migrate to travis-ci.com or GitHub Actions.

@ota-meshi
Copy link
Contributor Author

I opened a PR that uses GitHub Actions #117.

@michaelficarra michaelficarra merged commit 365ed15 into estools:master Feb 2, 2021
@ota-meshi ota-meshi deleted the custom-visitor-keys branch February 2, 2021 21:09
@brettz9
Copy link
Contributor

brettz9 commented Feb 5, 2021

Great to have this merged. Could we get an npm release too for it?

@michaelficarra
Copy link
Member

@brettz9 Published 1.4.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Add support for custom visitor keys.
3 participants