-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Add jsdoc support for @public/@private/@protected #35731
Conversation
move to utilities, make the right things call it
1. Update the API baselines with the new functions. 2. Do not check for @public/etc during parsing, because parent pointers aren't set, so non-local tags will be missed; this wrong answer will then be cached.
@@ -1476,7 +1473,7 @@ namespace ts { | |||
} | |||
|
|||
function maskModifierFlags(node: Node, modifierMask: ModifierFlags = ModifierFlags.All ^ ModifierFlags.Public, modifierAdditions: ModifierFlags = ModifierFlags.None): ModifierFlags { | |||
let flags = (getModifierFlags(node) & modifierMask) | modifierAdditions; | |||
let flags = (getEffectiveModifierFlags(node) & modifierMask) | modifierAdditions; |
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 the only substantive change in this file; all the others just eliminate checks that are already performed in ensureType
.
@typescript-bot perf test this |
Heya @sandersn, I've started to run the perf test suite on this PR at 62a568d. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
@sandersn Here they are:Comparison Report - master..35731
System
Hosts
Scenarios
|
src/compiler/utilities.ts
Outdated
@@ -4128,6 +4128,19 @@ namespace ts { | |||
return flags; | |||
} | |||
|
|||
export function getEffectiveModifierFlags(node: Node) { | |||
const flags = getModifierFlags(node); | |||
if (!!node.parent && isInJSFile(node)) { |
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.
Shouldn't you just integrate this into getModifierFlagsNoCache
? (Thus simultaneously caching the result and preventing the need for a separate getEffectiveModifierFlags
function) It already has special logic for isInJSDocNamespace
stuff, so...
Then move jsdoc modifier tag checks into getModifierFlagsNoCache where they should be. The jsdoc checks are skipped when the parent is undefined. There are 3 cases when this can happen: 1. The code is in the parser (or a few places in the binder, notably declareSymbol of module.exports assignments). 2. The node is a source file. 3. The node is synthetic, which I assume to be from the transforms. It is fine to call getModifierFlags in cases (2) and (3). It is not fine for (1), so I removed these calls and replaced them with simple iteration over the modifiers. Worth noting: Ron's uniform node construction PR removes these calls anyway; this PR just does it early.
OK, the @typescript-bot perf test this |
Heya @sandersn, I've started to run the perf test suite on this PR at f30f399. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
@sandersn Here they are:Comparison Report - master..35731
System
Hosts
Scenarios
|
Yep, there is now no perf change from this PR. |
} | ||
declare class Privacy { | ||
/** @private */ | ||
private constructor; |
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 a parse error in the playground. This test isn't flagging it because the input code has errors, so output validity isn't checked. You should split erroring/non-erroring cases into separate tests.
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.
Oh, I'd gotten so used to errors and symbol baselines co-existing I forgot that errors and declarations didn't. I'll create a separate declaration emit test.
1. Emit protected for protected, which I missed earlier. 2. Emit a constructor, not a property named "constructor". 3. Split declaration emit tests out so that errors are properly reported there.
All right, declaration emit, and its tests, are fixed now. |
Not clear what to do about type-space references to readonly, like /** @typedef {Object} x
* @readonly @property {number} foo
*/ But that's not in the jsdoc tool and not supported by closure as far as I know, so it's fine not to support it. |
How does this handle cases like this? What takes precedence? /** @private */
public getFoo(): string { /* ... */ } Background: I've been using this for documentation generation, for members that I'm using internally (in other subpackages of the same monorepo), but that shouldn't be included in the documentation. |
@d-fischer in TS files Jsdoc does not affect types. In JS files you can't use the |
That's all I needed to know, thanks! |
They should work exactly as they do in typescript.
Fixes #14009
This adds a potentially expensive check when retrieving modifiers, but it should only apply to JS, and modifiers are usually cached, so I don't think the impact will be that great.
In any case it should be completely unobservable from our current performance suite. I'll double-check that.