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

use jsdoc-type-pratt-parser #736

Merged
merged 13 commits into from
May 30, 2021
Merged

Conversation

simonseyock
Copy link
Contributor

This is a proof of concept for using jsdoc-type-pratt-parser instead of jsdoctypeparser.

There is one test failing. It expects this type expression to be invalid: module:abc#event:foo-bar. I don't exactly see why this should be the case, but I have to admit that I do not fully understand the event keyword in the jsdoc name paths. The value is accepted by jsdoc/catharsis.

The permissive mode is not implemented in jsdoc-type-pratt-parser as there are no unambiguous results for certain expressions. I created a small helper function for this. Maybe I redesign the interface of jsdoc-type-pratt-parser a little to integrate this kind of helper into the library.

@simonseyock
Copy link
Contributor Author

I introduced a tryParse function into jsdoc-type-pratt-parser and used that as a replacement for the permissive mode.

@brettz9
Copy link
Collaborator

brettz9 commented May 16, 2021

This is a proof of concept for using jsdoc-type-pratt-parser instead of jsdoctypeparser.

Holy cow!

A truly impressive amount of work. And what appears to be a great validation of the Pratt parser strategy.

As mentioned, I need a little rest time I think before exploring this, but this is really fantastic.

There is one test failing. It expects this type expression to be invalid: module:abc#event:foo-bar. I don't exactly see why this should be the case, but I have to admit that I do not fully understand the event keyword in the jsdoc name paths. The value is accepted by jsdoc/catharsis.

If it's accepted by catharsis, great. I think this test might have been just my testing to ensure the jsdoctypeparser grammar was being used for validation of some nuanced types, which in this case insisted on what the PEG grammar called a MemberName and which when unquoted only allowed for JsIdentifier (which wouldn't allow hyphens in the event name).

So, no problem dropping this test.

The permissive mode is not implemented in jsdoc-type-pratt-parser as there are no unambiguous results for certain expressions. I created a small helper function for this. Maybe I redesign the interface of jsdoc-type-pratt-parser a little to integrate this kind of helper into the library.

If it doesn't end up adding too much complexity, that sounds nice because some do seem to want to use our tooling just for semantics or for flexible tools. But we've already made explicit hints about the dangers of this mode so it wouldn't be the end of the world I think to drop it if it wasn't relatively easy to support (I see tsdoc mentions lax and strict modes on their site--more on tsdoc below).

Btw, FWIW, I am wondering whether we should begin defaulting with "typescript" mode given its meeting needs which jsdoc has yet to support as far as I'm aware (e.g., with import() and the ability to work with detecting third party declaration files), that it can nevertheless meet the needs of plain JS, and that it works with robust tooling like VS Studio (not a user myself and don't want to pin behavior on tools, but I think we should support what works best in the ecosystem / based on what people are using).

I'm not well familiar with whether the various documentation tools (including typedoc and jsdoc-to-markdown) have any problems processing the various modes, if not in failure compiling then in failing to link import() for example.

I can raise this in a separate issue, but figured you might have some thoughts even if you were not deeply familiar with all the tooling support either.

Btw, one project I imagine you really would want to check out if you haven't (and if I haven't mentioned it already) is https://tsdoc.org/ . Haven't dug in too deeply myself, but seems like the kind of rigorous ambition we need (and the kind of rigor you have apparently already been applying to your parser).

The community would really benefit I think though from a third-party standardized JSDoc specification collection/documentation site similar to this but which, while supporting TypeScript, was oriented in such a way as to make clear that it was not specific to TypeScript users. I think the official JSDoc site, while still under development and the ideal candidate for this, is simply not as responsive to more precise specification and third party concerns to meet the needs of the full community which works on top of JSDoc, while the TypeScript-based efforts I think may scare away some who either don't know TypeScript, or prefer to work with plain JSDoc + JavaScript (JJ).

Perhaps such efforts can also lead to what I have elsewhere referred to as the goal of building up JJ with enough precision so as to be compileable directly or indirectly through TypeScript into Web Assembly (and validatable as with TypeScript).

Btw, though it doesn't have much traffic (and I wouldn't be able to support it much anyways), you might nevertheless be interested in our Discord chat server which was intended to be an open space for jsdoc projects beyond our own: #601 .

Anyways, exciting work, and look forward to reviewing when I feel I may have a good enough block of time to review--but it looks like you've already taken the pain out of our job, given how smoothly you've oriented the tool to integrate!

@brettz9
Copy link
Collaborator

brettz9 commented May 18, 2021

To fully remove the jsdocttypeparser dependency, we will also need to modify https://github.com/es-joy/jsdoccomment to use your library.

In iterateJsdoc.js of eslint-plugin-jsdoc here, we are importing commentHandler from jsdoccomment which allows our contexts options in various rules to accept objects with a comment property which accept selectors against JSDoc AST/JSDoc Type AST--as is useful for those not using a full-blown comment-aware parser like our experimental https://github.com/brettz9/jsdoc-eslint-parser .

You can see this comment: property in action in the requireDescription.js test file as it has some comment: selectors, e.g., this one which has a JSDoc type.

The real work of jsdoccomment relative to prepping the jsdoctypeparser types for use as ESLint AST is within this small file.

This file basically just:

  1. Changes all jsdoctypeparser node types to be prefixed with JSDocType and camel-cased, e.g., JSDocTypeUnion. The idea is that if this is mixed in along with JavaScript (or TypeScript), it will be properly namespaced and visually distinct from other types.
  2. Converts the jsdoctypeparser types to CamelCase (as per the ESTree convention which ESLint and other tools like generic ESTree traversers/query engines use).
  3. Builds (camel-cased) VisitorKeys for jsdoctypeparser so that traversers know which properties of a given node type are descendable (e.g., INTERSECTION is converted to JSDocTypeIntersection and specifies left and right as its traverse-descendable properties).

@simonseyock
Copy link
Contributor Author

Btw, FWIW, I am wondering whether we should begin defaulting with "typescript" mode given its meeting needs which jsdoc has yet to support as far as I'm aware (e.g., with import() and the ability to work with detecting third party declaration files), that it can nevertheless meet the needs of plain JS, and that it works with robust tooling like VS Studio (not a user myself and don't want to pin behavior on tools, but I think we should support what works best in the ecosystem / based on what people are using).

I'm not well familiar with whether the various documentation tools (including typedoc and jsdoc-to-markdown) have any problems processing the various modes, if not in failure compiling then in failing to link import() for example.

I am in favour of defaulting to typescript. I have to say that I do not have to much familiarity with jsdoc tools as well. I just know that jsdoc itself does not support import() which is already annoying at least.

Btw, one project I imagine you really would want to check out if you haven't (and if I haven't mentioned it already) is https://tsdoc.org/ . Haven't dug in too deeply myself, but seems like the kind of rigorous ambition we need (and the kind of rigor you have apparently already been applying to your parser).

The community would really benefit I think though from a third-party standardized JSDoc specification collection/documentation site similar to this but which, while supporting TypeScript, was oriented in such a way as to make clear that it was not specific to TypeScript users. I think the official JSDoc site, while still under development and the ideal candidate for this, is simply not as responsive to more precise specification and third party concerns to meet the needs of the full community which works on top of JSDoc, while the TypeScript-based efforts I think may scare away some who either don't know TypeScript, or prefer to work with plain JSDoc + JavaScript (JJ).

Yeah that would be really nice. I would have loved something like that during development. I learned quite a bunch while developing this, but the documentation by jsdoc is really thin, the documentation on closure is better, but also not great. For typescript it is good, but also hard to find certain details. In the end I tried out many expression directly on the compilers.

@brettz9 brettz9 force-pushed the jsdoc-type-pratt-parser branch from b45cc3b to 918df38 Compare May 24, 2021 12:23
@simonseyock simonseyock changed the title proof of concept: jsdoc-type-pratt-parser use jsdoc-type-pratt-parser May 30, 2021
@brettz9 brettz9 force-pushed the jsdoc-type-pratt-parser branch from b9d7485 to c19d57e Compare May 30, 2021 13:39
@brettz9 brettz9 merged commit dc641cc into gajus:master May 30, 2021
@gajus
Copy link
Owner

gajus commented May 30, 2021

🎉 This PR is included in version 35.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants