-
-
Notifications
You must be signed in to change notification settings - Fork 3
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 JSONLanguage#visitorKeys #4
Conversation
Release-As: 0.1.0
@@ -9,6 +9,7 @@ | |||
|
|||
import { parse } from "@humanwhocodes/momoa"; | |||
import { JSONSourceCode } from "./json-source-code.js"; | |||
import { visitorKeys } from "@humanwhocodes/momoa"; |
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 should update the dependency to ^3.1.1
in package.json.
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 is so weird. I know I updated this locally and tested, and this is the second time you've pointed out that the version hasn't been updated in package.json
. 🤔
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.
Actually, it is updated in package.json
but for some reason git isn't included it in the branch.
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.
It works if 3.1.1
is locally installed, which in most cases would be the case after doing npm install
since it's the latest in-range version, but to be sure it should be set in package.json.
When I tested this, it turned out that things for which this matters don't actually work (e.g., selectors like |
Ah interesting. Yeah, I don't think we can assume every AST type will have a |
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.
LGTM, thanks!
Not intentional. Need to add that to the new repo template. |
Merging now as this change is correct for this package. I am working on fixing |
Prerequisites checklist
What is the purpose of this pull request?
Added
JSONLanguage#visitorKeys
.What changes did you make? (Give an overview)
JSONLanguage#visitorKeys
JSONLanguage#visitorKeys
is properly createdRelated Issues
Is there anything you'd like reviewers to focus on?
Please double check that the following line is included in the merge commit body to ensure we reset the package version number:
Release-As: 0.1.0