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

Make JSDoc skipping public, plus more configurable #55739

Merged
merged 26 commits into from Sep 21, 2023

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Sep 13, 2023

For #52959.

This is a follow-up to #52921.

This changes the way we do the new JSDoc skipping, instead replacing it with a publicly-available option which lets you control how much info you want. Specifically, you can specify:

  • JSDocParsingMode.ParseAll, the default, which is to keep parsing JSDoc and including it in the tree. This is the default used by tsserver, services, etc, where we definitely need all of that info.
  • JSDocParsingMode.ParseForTypeErrors, which only parses JSDoc tags which have a semantic meaning to the checker which impact error calculation, that is, any JSDoc in a non-TS/TSX file, or any JSDoc containing @see or @link. This is the new default when calling from tsc, and would be a good default for anyone orchestrating TS builds, e.g. heft, nx.
  • JSDocParsingMode.ParseForTypeInfo, if you only query type info, but never care about its errors. This parses JSDoc in JS files, but doesn't in TS files at all (as they are never used for type info there). This is good for something like ts-eslint with type info (which does not surface errors).
  • JSDocParsingMode.ParseNone, which always skips JSDoc, no matter what. This is good for those who only want the parser, e.g. formatters like prettier, ts-eslint without type information, transpileModule, maybe others. Set this at your own risk.

To use this when calling createSourceFile, set jsDocParsingMode on the create options:

const sourceFile = ts.createSourceFile(
    filename,
    content,
    { languageVersion: ts.ScriptTarget.ESNext, jsDocParsingMode: ts.JSDocParsingMode.ParseNone },
);

To use this when constructing a host, set jsDocParsingMode:

const host = ts.createCompilerHost(options);
host.jsDocParsingMode = ts.JSDocParsingMode.ParseForTypeInfo;
// or
const host = ts.createIncrementalCompilerHost(options);
host.jsDocParsingMode = ts.JSDocParsingMode.ParseForTypeErrors;
// or
const host = ts.createWatchCompilerHost(...);
host.jsDocParsingMode = ts.JSDocParsingMode.ParseForTypeErrors;

FYI @fisker @bradzacher @JoshuaKGoldberg @JamesHenry @dmichon-msft

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Sep 13, 2023
@typescript-bot
Copy link
Collaborator

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up.

@jakebailey
Copy link
Member Author

Bleh, there's that dprint ternary annoyance again that Sheetal noted (https://github.com/microsoft/TypeScript/actions/runs/6178742090/job/16772520512?pr=55739), maybe that's dprint/dprint-plugin-typescript#450 ?

@bradzacher
Copy link
Contributor

Copying from the other PR

I think my main concern is that people will want to read out the .jsDoc properties by asking typescript-eslint for the original AST nodes (e.g. #55139), and then find that the info was never populated, or similar. I also wanted to try and make .jsDoc lazy and only create it when accessed (since we have the text anyway), but since JSDoc is bound by the binder, you can't really do that.

I think that this is going to be the rare case, if it even exists across the ecosystem.
If this is a configurable thing then we can easily set it up to be configurable on our side too - eg a user can pass parserOptions.tsJSdoc or something in their eslint config if a plugin needs it (and plugins can add it to their recommended config).

The vast majority of the ecosystem would just get faster-by-default lint runs which would be a big win for everyone!

@jakebailey
Copy link
Member Author

On that front, I probably need to figure out a way to plumb this through ProjectService too.

@jakebailey jakebailey marked this pull request as ready for review September 15, 2023 18:22
@jakebailey
Copy link
Member Author

Marking as ready for review, though I need to double check that I have plumbed this up through createProgram and so on.

@jakebailey
Copy link
Member Author

It turns out that I missed a lot of places that should plumb this. This will be tricky. ☹️

@jakebailey jakebailey marked this pull request as draft September 15, 2023 18:34
src/services/types.ts Outdated Show resolved Hide resolved
src/compiler/tsbuildPublic.ts Outdated Show resolved Hide resolved
@@ -393,16 +394,16 @@ export function computeCommonSourceDirectoryOfFilenames(fileNames: readonly stri
return getPathFromPathComponents(commonPathComponents);
}

export function createCompilerHost(options: CompilerOptions, setParentNodes?: boolean): CompilerHost {
return createCompilerHostWorker(options, setParentNodes);
export function createCompilerHost(options: CompilerOptions, setParentNodes?: boolean, jsDocParsingMode?: JSDocParsingMode): CompilerHost {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think we should be threading in this to defaults. User of this API can always call thius method and override getSourceFile on their own .. there is nothing special here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that not providing this parameter will make it very inconvenient; this function and createCompilerHostWorker are what call createGetSourceFile, which is internal and non-trivial, and then wrap it again in another closure. Setting the JSDoc parsing mode is a lot like setting a default setParentNodes which this function already does.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally speaking I think this means that anywhere we can expect to find setParentNodes, we probably need to be expecting jsDocParsingMode.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. Generally i like to go with conservative approach and avoid having to add additional surface area that we have to keep maintaining. This is ok i guess though , user can just create source file from file text easily to override but ok.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the option is fed in via the CreateSourceFileOptions, rather than a top-level field, callers will already have the opportunity to request it on a per-file basis when making the file, rather than needing to set it on the host as a whole.

src/compiler/watch.ts Outdated Show resolved Hide resolved
src/compiler/watch.ts Outdated Show resolved Hide resolved
src/compiler/watchPublic.ts Outdated Show resolved Hide resolved
src/compiler/watchPublic.ts Outdated Show resolved Hide resolved
src/services/services.ts Outdated Show resolved Hide resolved
src/services/types.ts Show resolved Hide resolved
@jakebailey
Copy link
Member Author

FYI @nonara; in executeCommandLine.ts, there's a new global defaultJSDocParsingMode; in tsc here that's set to ParseForTypeErrors, but it's likely that someone's plugin will actually need this to be reset to ParseAll in order to get all of the JSDoc nodes they expect. e.g. I suspect typia reads JSDoc, somehow. Probably others. Just something to watch out for!

* This will always parse JSDoc in non-TS files, but only parse JSDoc comments
* containing `@see` and `@link` in TS files.
*/
ParseForTypeErrors,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last minute bikeshedding; ParseForErrors? Technically, this covers grammar checks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But, not actually sure we do grammar checks which relate to JSDoc. I assume not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in TS files, no. Just in JS files.

@nonara
Copy link

nonara commented Sep 21, 2023

Thanks for the heads up!

@sandersn sandersn added this to Not started in PR Backlog Sep 21, 2023
@sandersn sandersn moved this from Not started to Needs merge in PR Backlog Sep 21, 2023
@jakebailey
Copy link
Member Author

Alright, seems like everything is alright; I'm going to merge so we can get this in for 5.3. Thank you everyone for the feedback, hopefully it works out (and tell me if it doesn't).

@jakebailey jakebailey merged commit a3c5d5d into microsoft:main Sep 21, 2023
19 checks passed
PR Backlog automation moved this from Needs merge to Done Sep 21, 2023
@jakebailey jakebailey deleted the skip-jsdoc-2 branch September 21, 2023 22:31
snovader pushed a commit to mestro-se/TypeScript that referenced this pull request Sep 23, 2023
samchon added a commit to samchon/typia that referenced this pull request Nov 8, 2023
Test new property `CompilerHost.jsDocParsingMode` on playground following below guide:

microsoft/TypeScript#55739
samchon added a commit to samchon/nestia that referenced this pull request Nov 8, 2023
Since TypeScript v5.3 update, TypeScript compiler no more analyzes `JsDocComment` as default to boost up compilation speed by not parsing the JS Doc Comments.

Therefore, to parepare the future TypeScript v5.3 release, I've updated `@nestia/sdk` to configure the `jsDocParsingMode` to parse every comments manually.

microsoft/TypeScript#55739
@samchon
Copy link

samchon commented Nov 8, 2023

Thanks for detailed documentation.

samchon added a commit to samchon/typia that referenced this pull request Nov 22, 2023
As TypeScript starts skipping parsing `JSDocComment`s since TypeScript v5.3 update but it is possible to revive the `JSDocComment`s' parsing, by hacking the `jsDocParsingMode` value of `tsc.js`, I've decided to print a warning message from `typia` when the `jsDocParsingMode` value is not hacked.

Therefore, it is possible to running the `typia` as before even when the `jsDocParsingMode` is not hacked, and user can ignore the warning message if he (or she) does not use any comment tags or not generating JSON schema with description comments.

- Related issue: microsoft/TypeScript#55739
samchon added a commit to samchon/nestia that referenced this pull request Nov 26, 2023
Since TypeScript v5.3 update, `tsc` no more parses `JSDocComment`s. Therefore, Therefore, `typia` and `@nestia/core` also cannot utilize those `JSDocComment` related features too, especially "Comment Tags" and "JSON schema generator".

However, in relation to this, the upgrade of `ts-patch` continues to be delayed, and I still don't know how long the delay would be continue. Furthermore, there're some `typia`/`nestia` users urging to resolve the `peerDependencies` of `typia`/`nestia` that blocking the TypeScript v5.3 update. Therefore, before the `ts-patch` being prepared, I've decoded to provide `typia`'s own solution for a while. It is the new CLI command `npx typia patch`, and `nestia` also adapts it (`npx nestia setup` command performs it).

Also, if the `defaultJSDocParsingMode` value not being patched, `typia` will generate an warning message of TypeScript compiler API. For reference, as it is an warning message, it does not interrupt the TypeScript compilation like the compilation error case. If there're some `typia`/`nestia` users never using "Comment Tags" or "JSON schema generator" at all, they don't need to run the CLI command. This is not mandatory command, but just optional command.

Of course, when `ts-patch` being updated, this CLI command would be disabled immediately, if the installed `ts-patch` version is the latest one.

Related issues:
  - samchon/typia#883
  - microsoft/TypeScript#55739
  - nonara/ts-patch#134
samchon added a commit to samchon/payments that referenced this pull request Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

None yet