Add jsdoc support for @public/@private/@protected #35731
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
sandersn
commented
Dec 17, 2019
| @@ -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.
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
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
weswigham
reviewed
Dec 17, 2019
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.
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. |
weswigham
reviewed
Dec 18, 2019
weswigham
reviewed
Dec 18, 2019
weswigham
reviewed
Dec 18, 2019
| } | ||
| declare class Privacy { | ||
| /** @private */ | ||
| private constructor; |
There was a problem hiding this comment.
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.
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. |
andrewbranch
approved these changes
Dec 18, 2019
|
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. |
DanielRosenwasser
added
the
Update Docs on Next Release
|
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! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.