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

Feature Request: get target node type from selector #38

Open
nzakas opened this issue May 16, 2014 · 5 comments
Open

Feature Request: get target node type from selector #38

nzakas opened this issue May 16, 2014 · 5 comments

Comments

@nzakas
Copy link
Contributor

nzakas commented May 16, 2014

I'm looking at using esquery in ESLint, and it looks very close to what I need for the implementation I have in mind. The one thing that is missing is a way to figure out the target node type of a given selector. Basically, something that can take the selector AST and returns:

"*" => "*"
"FunctionDeclaration > VariableDeclarator" => "VariableDeclarator"
":not(FunctionDeclaration)" => "*"

Basically, my implementation will look like:

var queryAST = esquery.parse(key);
var targetType = esquery.getTargetType(queryAST);

eslint.on(targetType, function(node) {
    If (esquery.matches(node, queryAST, parents)) {
        // apply rule
    }
}
@michaelficarra
Copy link
Member

I'm not sure I understand your use case. Can you explain what eslint.on is doing with the first parameter? Have you looked at esdispatch yet? I designed its interface with tools like eslint in mind. It seems pretty similar to your proposal here.

@nzakas
Copy link
Contributor Author

nzakas commented May 19, 2014

Yes, I've looked at esdispatch, and I don't like the approach of checking every node against every selector. I'd also rather have a direct dependency on esquery rather than having it be one step away in the dependency tree.

What I want is a way to narrow down which selectors should be checked against which nodes rather than always checking every one.

eslint emits events for each node type, so "FunctionDeclaration", for example. Currently rules, listen for those events and then do their processing. With esquery, that would change so that the event the rule listens for is the target type of the selector. Then, it checks to see if the whole selector matches before applying the rule. This allows us to keep the core of eslint working the same way (emitting events for node types) and just slightly change how that affects rules.

@nzakas
Copy link
Contributor Author

nzakas commented Jun 4, 2014

Is this going to happen? I can make this functionality myself, it just seems like a nice fit with this library to bundle it.

And is esquery still under development? It looks like there hasn't been much movement recently.

@michaelficarra
Copy link
Member

So I think you're confused about the matching process. Determining the "target type" is as expensive as doing a full match. Matching A > B fails fast if the tested node's type is not B. If it is, it continues to check if B's parent is A. Are you concerned that won't be performant?

edit: By the way, since esdispatch knows all of the selectors, we can build an optimised automaton to avoid performing the same test more than once.

I'm still happy to work on esquery, but I have no incentive right now. I don't run into any of the currently open issues myself, they're pretty much all minor, and there's no big widely-used project relying on it yet.

@nzakas
Copy link
Contributor Author

nzakas commented Jun 5, 2014

Yes, I don't want to have to run every query on every node - that seems
like wasted work when we know most of the time, the query won't match.

I may be missing something, but it seems like determining the target node
type would be pretty fast since you're just walking the parsed query
structure to get to the end node without actually doing a match or walking
the Esprima AST. Am I wrong?

You have a bit of a chicken-and-egg problem. I'm afraid of relying on
esquery because there's no activity on it, and you're not updating it
because no one is relying on it. So, if you were to be committed to
maintenance than I would commit to using it. :)

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

No branches or pull requests

2 participants