-
Notifications
You must be signed in to change notification settings - Fork 91
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
Update deps and adds some useful API documentation #49
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're going to need more comprehensive tests for :has
. I've left a bunch of comments and will do a follow-up review once they're addressed.
API usage.md
Outdated
|
||
*Install (via npm for Node.js)* | ||
|
||
`npm i esquery --save` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I strongly prefer installation documentation to use npm install ...
. There's no reason to use shorthand in documentation.
API usage.md
Outdated
```js | ||
const esquery = require('esquery'); | ||
|
||
const conditional = "if (x === 1) { foo(); } else { x = 2; }" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use single quotes or double quotes, but not both.
API usage.md
Outdated
`[name="x"]:function` - function named `x` | ||
`[name="foo"]:declaration` - declaration named `foo` | ||
|
||
## Complex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Combinators
API usage.md
Outdated
- adjacent sibling selector (`+`) | ||
- general sibling selector (`~`) | ||
|
||
Please see [javascript: expressions-vs-statements](http://www.2ality.com/2012/09/expressions-vs-statements.html) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how this is related. Instead, one should see the estree specification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might also want to point them to the CSS documentation because these combinators are both syntactically and semantically identical.
API usage.md
Outdated
|
||
Please see [javascript: expressions-vs-statements](http://www.2ality.com/2012/09/expressions-vs-statements.html) | ||
|
||
`IfStatement > BinaryExpression` - `if` statement followed by a [binary expression](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Expressions_and_Operators) fx `3+4` or `x*y` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, a BinaryExpression
nested directly within an IfStatement
.
API usage.md
Outdated
|
||
`VariableDeclaration ~ IfStatement` | ||
|
||
`var` declaration with sibling `if` statement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if
statement with sibling var
declaration. The if
statement is the target.
API usage.md
Outdated
|
||
### Fields | ||
|
||
You can also query on the [Mozilla Parser API](https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Parser_API) fields directly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This link should be updated to estree.
API usage.md
Outdated
|
||
### Subject | ||
|
||
`!IfStatement Identifier` - any not an If statement with an Identifier under, such as `const x = 3` but not `if (x == 2)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any not an If statement with an Identifier under
any if
statement with one or more nested Identifier
API usage.md
Outdated
|
||
`!IfStatement Identifier` - any not an If statement with an Identifier under, such as `const x = 3` but not `if (x == 2)` | ||
|
||
`!* > [name="foo"]` all nodes but those where the immediate child is a node namded `foo` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
namded -> named
API usage.md
Outdated
- `!:matches(*) > [name="foo"]` | ||
- `!:not(BlockStatement) > [name="foo"]` | ||
- `![left.name="x"][right.value=1]` | ||
- `* !AssignmentExpression` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a productive example. What is it trying to show off?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do whatever it takes to add sufficient docs for how to use the API. I tried my best using the tests cases for inspiration.
@kristianmandrup I hope you update the PR based on the feedback and help get this merged in - I found this document extremely helpful in understanding how to use esquery! |
Hey! I would be grateful if you would get it merged. Thanks! |
I made all the fixes as per your suggestions. Please review this PR again and let's make a good API documentation :) |
@kristianmandrup It seems your branch has fallen behind. You should rebase on this repo's |
@@ -1,7 +1,7 @@ | |||
{ | |||
"name": "esquery", | |||
"preferGlobal": false, | |||
"version": "0.4.0", | |||
"version": "0.4.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't bump the version. We'll bump it appropriately during a release.
Please review and merge ;)