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

feat: add support for :scope selector #23

Open
petebacondarwin opened this issue Jul 17, 2018 · 25 comments
Open

feat: add support for :scope selector #23

petebacondarwin opened this issue Jul 17, 2018 · 25 comments

Comments

@petebacondarwin
Copy link

See https://developer.mozilla.org/en-US/docs/Web/CSS/:scope

This selector would allow queries to limit the matching to only the direct children of the AST on which the query is run.

For example, in the following code:

const x = function() {
  function y() {
    return 'bar';
  }
  return 'foo';
};

How do you query for just the outer function's return statement? Currently the query ReturnStatement will return both return statements even if it is run on the Block node that is the body of the outer function.

With the :scope selector you would be able to use :scope > ReturnStatement and then run that on the body of the outer function to only return the outer ReturnStatement.

ESQuery has a PR for adding this already estools/esquery#61

@petebacondarwin
Copy link
Author

Of course since tsquery uses esquery to parse the query strings, it will be tricky to fix this until esquery supports it too...

@phenomnomnominal
Copy link
Owner

Yeah that's a great idea, and their implementation looks solid. I'll give the ESQuery team a little while to see if they respond to the comments, but then will definitely look at moving the parser into TSQuery (hopefully temporarily)

@petebacondarwin
Copy link
Author

Great! Happy to create PR(s) as necessary. Just give me the nod.

@phenomnomnominal
Copy link
Owner

I had a go at adding unit tests to the PR in ESQuery, but other tests were failing and I don't have time to dig into them right now 😕

@petebacondarwin do you reckon it's worth fixing it up their side, or should we try to do it directly in TSQuery?

@run1t
Copy link

run1t commented Nov 11, 2018

It would be very helpful to be able to use the :scope selector.
Any news about this ?

@run1t
Copy link

run1t commented Nov 12, 2018

I added some very basic unit tests to the PR in ESQuery, where should I do the pull requests ?
Do I have to create another one on https://github.com/estools/esquery or do I have to pull request directly in this one: https://github.com/jupenur/esquery ?

If you have anothers tests in mind I'll be glad to add them.
You can see my branch here: https://github.com/run1t/esquery/tree/scope-root-tests

@phenomnomnominal
Copy link
Owner

Thank you so much for your work on this! I'm not really sure how the PR should work, you could just do it against esquery and see what happens? Might be more direct that way. I guess they'll tell you if it doesn't work right.

I'd probably suggest adding another test for the use case mentioned in the original issue here as well, just to make sure we're solving the original request?

@run1t
Copy link

run1t commented Nov 12, 2018

I'll add the test for the use case described above and I'll create a new pull request against esquery directly 😃

@run1t
Copy link

run1t commented Nov 12, 2018

If the pull request is accepted and a new version is published, what would be the next steps (beside updating the ESquery version in package.json)?

@phenomnomnominal
Copy link
Owner

Just updating the version and then publishing it 😊

@run1t
Copy link

run1t commented Nov 12, 2018

Unfortunately it does not seem so easy 😢

but I have this error when I run the tests:

tsquery: › tsquery - :has: › should find any nodes with multiple attributes

    Unknown selector type: scope

      33 |     }
      34 |
    > 35 |     throw new Error(`Unknown selector type: ${selector.type}`);
         |           ^
      36 | }
      37 |

The tests below are failing with the same error:

  • tsquery: › tsquery - :has: › should find any nodes with multiple attributes
  • tsquery: › tsquery - :has: › should find any nodes with one of multiple attributes
  • tsquery: › tsquery - :has: › should handle chained :has selectors
  • tsquery: › tsquery - :has: › should handle nested :has selectors

Do you have any ideas on why it's failing ?

@run1t
Copy link

run1t commented Nov 12, 2018

It's seems that a new matcher is needed in this file:

// Dependencies:
import { TSQueryMatchers } from '../tsquery-types';
// Matches:
import { attribute } from './attribute';
import { child } from './child';
import { classs } from './class';
import { descendant } from './descendant';
import { field } from './field';
import { has } from './has';
import { identifier } from './identifier';
import { matches } from './matches';
import { not } from './not';
import { nthChild, nthLastChild } from './nth-child';
import { adjacent, sibling } from './sibling';
import { wildcard } from './wildcard';
export const MATCHERS: TSQueryMatchers = {
adjacent,
attribute,
child,
compound: matches('every'),
'class': classs,
descendant,
field,
'nth-child': nthChild,
'nth-last-child': nthLastChild,
has,
identifier,
matches: matches('some'),
not,
sibling,
wildcard
};

@phenomnomnominal
Copy link
Owner

Ah yep, too right. Once their PR gets merged and released I can port over the matcher and the tests too.

@petebacondarwin
Copy link
Author

@run1t - I think you can test this locally more easily with npm link (or yarn link), rather than resorting to publishing your own version.

@run1t
Copy link

run1t commented Nov 13, 2018

@petebacondarwin - Thanks for the advice !

@run1t
Copy link

run1t commented Nov 16, 2018

I was trying to implement the :root and :scope selector. I got :root working. But when I was working on implementing the :scope selector I didn't understand the differences between both.

I looked at the implementation in ESQuery, and they seem to differentiate the two.
https://github.com/run1t/esquery/blob/1f3cfdb0e376d5a0aa83a36c793d5f5649c7abda/esquery.js#L190-194

Do someone know the goal of this ? And why there is a scope parameter added here ?: https://github.com/run1t/esquery/blob/1f3cfdb0e376d5a0aa83a36c793d5f5649c7abda/esquery.js#L50

@petebacondarwin
Copy link
Author

I find this explanation of :scope the most useful: https://developer.mozilla.org/en-US/docs/Web/CSS/:scope

So in a CSS stylesheet there is only one scope, which happens to be the :root scope.
But when programmatically calling CSS path matching functions, the scope is the element from which you start matching.

I believe that the implementation of matches in esquery is designed to be called recursively. They must pass the scope node down to each recursive call so that they are all working with the same original scope.

So to summarize:

  • scope refers to the starting point of the query, which may be any node in the tree.
  • root is always refers to the node at the root of the tree

@phenomnomnominal
Copy link
Owner

TSQuery’s matcher implementation is almost identical to ESQuery, I’ve just split it up a bit more and renamed stuff. If you’d like to make a PR of where you’ve got too, I can try to give you advise on what to try next?

@run1t
Copy link

run1t commented Nov 16, 2018

@petebacondarwin Thanks for the explanation

So if we take this example:

function a() {
  function b() {
    return 'bar';
  }
  return 'foo';
};

If I understood correctly:

Assuming we are in the BLOCK of the function a:
  • Executing this query :root > FunctionDeclaration should return the function a
  • Executing this query :scope > FunctionDeclaration should return the function b

Am I right ?

@phenomnomnominal I'll make a PR later today. Thank you !

@petebacondarwin
Copy link
Author

That that is how I understand it. Moreover:

Assuming we are in the BLOCK of the function a:

  • Executing this query :root FunctionDeclaration should return function a and function b
  • Executing this query :scope FunctionDeclaration should only return function b

@run1t
Copy link

run1t commented Nov 17, 2018

@phenomnomnominal you can now take a look at the PR #37 😃
@petebacondarwin I added your use cases in the PR, Thanks !

@run1t
Copy link

run1t commented Nov 27, 2018

If esquery doesn't want to accept the pull request how should I do ?

@phenomnomnominal
Copy link
Owner

Do we know that they don’t? Let’s give them a little nudge first, and see what needs to happen for it to be merged.

@run1t
Copy link

run1t commented Nov 28, 2018

I hope they will ! :), but it's been 15 days and nobody responded on the pull request. So I was wondering, At which point we need to consider it will not be merged ? I'm new to open source so I don't really know what to expect. I need to be patient I guess !

@cancerberoSgx
Copy link

cancerberoSgx commented Apr 10, 2019

If I'm understanding this correctly, I think:root would be the same as SourceFile?. Example, SourceFile>FunctionDeclaration will select root function declarations while FunctionDeclaration will select all.Or an I missing something ?

About the question, you select the outer ReturnStatement like this: FunctionExpression >Block > * > ReturnStatement. Or more specific: SourceFile>FunctionExpression >Block > * > ReturnStatement.

The '*' would be a SyntaxList but seems is treated specially and you cannot mention it in the query (i.e this works: FunctionExpression >Block > SyntaxList, this won't work: FunctionExpression >Block > SyntaxList *)

About :scope , IMO, in the context of JavaScript it could be confused with a symbol scope. My two cents

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

4 participants