-
Notifications
You must be signed in to change notification settings - Fork 13
Offer semantic services alongside AST #24
Offer semantic services alongside AST #24
Conversation
Will fix the merge conflict on Monday (and push a couple more commits that I don't have access to right now), but the core approach will stay unchanged. |
Really exciting stuff, @uniqueiniquity! Thanks again for contributing, @DanielRosenwasser mentioned this might be coming up soon. I am sorry to have made more work for you when it comes to reconciling conflicts, but I unfortunately don't have scope to work on this during working hours right now, and have to seize any free-time opportunity I get (which happened to be this weekend). In #25 I have brute force (via a gazillion type assertions) converted the repo to finally use TypeScript for its own source. The idea being that I didn't want to make runtime changes/improvements along the way which might get lost within the sheer size of the PR. There can now be lots of smaller, more focused follow ups to improve the way type information flows through. I am sure a number of your improvements in this PR can further utilize that now too. Thanks so much again! |
That's totally fine! Of course I'm biased, but I'm more than happy to convert this to TypeScript. 😄 |
@uniqueiniquity Is this ready for review? |
@JamesHenry yes please! |
@@ -183,5 +257,13 @@ export { version }; | |||
const version = packageJSON.version; | |||
|
|||
export function parse(code: string, options: ParserOptions) { |
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 think it would be good to keep the parse() function as being as simple as possible:
parse(CODE, OPTIONS): AST
Maybe we could add an alternative method:
parseAndGenerateServices(CODE, OPTIONS): YOUR INTERFACE
(open to suggestions on the name)
Then the opt-in is even more explicit for consumers, and the parse function interface will match what other parsers in this space expose.
We did a similar thing for ESLint: we have parse and parseForESLint exposed in custom parsers.
WDYT?
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 could also put the parser services in the AST.
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.
That would make them harder to serialize though right?
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 fine with adding a second method for explicit opt in.
One thing that I'm not sure about in general is whether or not to always give the ts<->estree maps, since (as it stands) we always create them.
So this could give three levels:
- parse gives ast
- parseAndGenerate w/o project flag gives ast and maps
- parseAndGenerate w/ project flag gives ast, maps, and program
Does it make any sense to set it up that way? The TS AST does make it easier to do some things that don't involve semantic information, like finding the position of keywords within nodes.
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.
That would make them harder to serialize though right?
You’d just need to add a toJSON
method:
JSON.stringify({ foo: { toJSON: () => undefined } })
// => "{}"
Or is that not what you meant @JamesHenry?
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 think for this initial PR we could start with just two: all or nothing.
-
parse() will be as it is today, intended for consumers like prettier where we want an ESTree-compatible AST as quickly as possible (i.e. skip any work that isn't conversion related)
-
parseAndGenerateServices() which exposes everything (ast, maps, and program), intended for consumers like ESLint who want to consume semantic/type information for advanced rule creation.
By doing it this way all the new stuff is additive and we don't have to worry about changing the experience for existing consumers. If the new stuff is not quite right for the advanced use-case, we can then further refine it in follow up PRs.
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.
Makes sense to me. I've exposed a separate top-level function called parseAndGenerateServices
and made all of the additions (i.e. maps and program) only run if that new function is called and the project
option is given.
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.
See: #24 (comment)
@JamesHenry I've made the changes you requested, I think. Would love for you to take another look! |
Awesome, thanks so much again! |
🎉 This PR is included in version 5.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
General organization of changes:
project
, which takes either a single project file path or an array of project file paths (top-levelparser.js
)tsconfig-parser.js
)--fix
or when ESLint is used as a server, as it is in Visual Studio and Visual Studio Code) are tracked by the TS watch API.convert.js
, passed up throughast-converter.js
to top-level).convert.js
to avoid having to ensure parent pointers are set in the TS AST (that is to say, for efficiency).parser.js
)Note that the features added by this PR are entirely enabled or disabled (from the consumer's perspective) by the
project
option; the fallback behavior is the existing behavior.I also added test coverage to ensure the two-way mapping is consistent (i.e.
ts2es(es2ts(node)) = node
) and that the exposed checker is usable as a rule might expect.Map approach is based on eslint/typescript-eslint-parser#415