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.

Sign up for GitHub

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

Jump to bottom

On @api JSDoc tag #1949

Closed
aweebit opened this issue Aug 7, 2023 · 11 comments
Closed

On @api JSDoc tag #1949

aweebit opened this issue Aug 7, 2023 · 11 comments

Comments

@aweebit
Copy link
Contributor

aweebit commented Aug 7, 2023 •
edited

The @api JSDoc tag has been used in the library code since the initial commit. However, I have not been able to find any documentation on it. The fact its values are properly highlighted in VS Code is due to microsoft/vscode@9ad4bcd, which includes no explanation on why the highlighting was added. My best guess is that it was added just to cover de facto usage like that in Commander (see microsoft/vscode#189812).

The well-documented alternatives to @api private (which is currently used throughout the library) are @private and @access private. Should we use one of them instead?
@aweebit
Copy link
Contributor Author

aweebit commented Aug 7, 2023

Found the progenitor commit adding the highlighting later adopted by VS Code: 31d2a5f

My assumption was right. The commit description reads:

Not part of the official JSDoc spec, yet occasionally used in-the-wild.

@shadowspawn
Copy link
Collaborator

Happy to change to @private.

I had noticed @api was not official in past, but it was used so consistently right from the start I just left it! The atom link that you found that added the support was interesting history.

The main documentation used to be generated on the gh-pages branch from the JSDoc and published separately. It is probably the reason we have such good JSDoc coverage! That process wasn't being maintained, and we switched to the README for the docs. (#299 #988)

The utility used to extract the JSDoc from the code did explicitly support @api private: https://github.com/tj/dox

@aweebit
Copy link
Contributor Author

aweebit commented Aug 8, 2023

I found other usage examples and opened jsdoc/jsdoc#2066. Excited to see where it leads, but not expecting quick reaction since there has not been much activity in the repo recently.

This was referenced Aug 12, 2023
Closed
Closed
aweebit added a commit to aweebit/commander.js that referenced this issue Aug 17, 2023
@aweebit
Use @Package instead of @api private in JSDoc
cd4338b 
Fixes tj#1949
@aweebit
Copy link
Contributor Author

aweebit commented Aug 17, 2023

I've just realized @package should be used instead of @private or @api private because classes use each others' "private" methods all over the library.

@aweebit aweebit mentioned this issue Aug 17, 2023
Closed
@shadowspawn
Copy link
Collaborator

shadowspawn commented Aug 17, 2023 •
edited

@package is technically accurate, but I had never seen it and had to look it up to confirm the meaning. I think @private is much clearer for Commander users seeing the tag.

@aweebit
Copy link
Contributor Author

aweebit commented Aug 17, 2023 •
edited

@package is technically accurate, but I had never seen it and had to look it up to confirm the meaning.

Unfortunately, it is indeed quite uncommon (only 10k results in GitHub Search).

I think @private is much clearer for Commander users seeing the tag.

The problem is that if you check out shadowspawn/commander.js@caf721c and then replace all @api private occurrences by @private, you will see a bunch of TypeScript errors that were not show before because @api private is not valid JSDoc, and in particular not currently supported by the TypeScript flavor of JSDoc, but that could change in the future.

I don't think we should keep using @api private just so that we can avoid errors because actually, it is supposed to simply be a synonym for @private, even though it currently isn't in tools other than Dox.

@aweebit
Copy link
Contributor Author

aweebit commented Aug 17, 2023

And by the way, the TypeScript team strongly advocates for having public declarations even for private class members.

But again quite unfortunately, TypeScript does not have an accessibility modifier that would be matched by the JSDoc @package tag (although an issue proposing such a modifier has been open since 2015: microsoft/TypeScript#5228).

So if we follow the guidelines and add typings for all package-private stuff, nothing will stop library users from accessing it, and they will even see it suggested by IntelliSense. It really frustrates me that TypeScript is such a mess :(

@shadowspawn
Copy link
Collaborator

The problem is if you ... replace all @api private occurrences by @private, you will see a bunch of TypeScript errors that were not show before because @api private is not valid JSDoc

Oh, yes indeed. And to be fair, correct! I have been accessing some methods as "package" accessible rather than being strict. That raises the bar. Not sure whether I want to change anything.

@aweebit
Copy link
Contributor Author

aweebit commented Aug 18, 2023

Just found out TypeScript recognizes @internal as a way to omit stuff from being included in declaration files under the stripInternal option. Could be exactly what we need.

@shadowspawn
Copy link
Collaborator

I prefer sticking with JSDoc for the Javascript tags, so could use mixture of @package // internal use only and @private. (Disclaimer: I have not experimented with this yet.)

@shadowspawn shadowspawn mentioned this issue Sep 15, 2023
Merged
@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Sep 16, 2023
@shadowspawn shadowspawn removed the pending release Merged into a branch for a future release, but not released yet label Feb 3, 2024
@shadowspawn
Copy link
Collaborator

Released in Commander v12.0.0.

Vylpes pushed a commit to Vylpes/random-bunny that referenced this issue Apr 10, 2024
@Vylpes
Update dependency commander to v12 (#145)
ac8691b 
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [commander](https://github.com/tj/commander.js) | dependencies | major | [`^11.1.0` -> `^12.0.0`](https://renovatebot.com/diffs/npm/commander/11.1.0/12.0.0) |

---

### Release Notes

<details>
<summary>tj/commander.js (commander)</summary>

### [`v12.0.0`](https://github.com/tj/commander.js/blob/HEAD/CHANGELOG.md#1200-2024-02-03)

[Compare Source](tj/commander.js@v11.1.0...v12.0.0)

##### Added

-   `.addHelpOption()` as another way of configuring built-in help option (\[[#&#8203;2006](tj/commander.js#2006)])
-   `.helpCommand()` for configuring built-in help command (\[[#&#8203;2087](tj/commander.js#2087)])

##### Fixed

-   *Breaking:* use non-zero exit code when spawned executable subcommand terminates due to a signal (\[[#&#8203;2023](tj/commander.js#2023)])
-   *Breaking:* check `passThroughOptions` constraints when using `.addCommand` and throw if parent command does not have `.enablePositionalOptions()` enabled (\[[#&#8203;1937](tj/commander.js#1937)])

##### Changed

-   *Breaking:* Commander 12 requires Node.js v18 or higher (\[[#&#8203;2027](tj/commander.js#2027)])
-   *Breaking:* throw an error if add an option with a flag which is already in use (\[[#&#8203;2055](tj/commander.js#2055)])
-   *Breaking:* throw an error if add a command with name or alias which is already in use (\[[#&#8203;2059](tj/commander.js#2059)])
-   *Breaking:* throw error when calling `.storeOptionsAsProperties()` after setting an option value (\[[#&#8203;1928](tj/commander.js#1928)])
-   replace non-standard JSDoc of `@api private` with documented `@private` (\[[#&#8203;1949](tj/commander.js#1949)])
-   `.addHelpCommand()` now takes a Command (passing string or boolean still works as before but deprecated) (\[[#&#8203;2087](tj/commander.js#2087)])
-   refactor internal implementation of built-in help option (\[[#&#8203;2006](tj/commander.js#2006)])
-   refactor internal implementation of built-in help command (\[[#&#8203;2087](tj/commander.js#2087)])

##### Deprecated

-   `.addHelpCommand()` passing string or boolean (use `.helpCommand()` or pass a Command) (\[[#&#8203;2087](tj/commander.js#2087)])

##### Removed

-   *Breaking:* removed default export of a global Command instance from CommonJS (use the named `program` export instead) (\[[#&#8203;2017](tj/commander.js#2017)])

##### Migration Tips

**global program**

If you are using the [deprecated](./docs/deprecated.md#default-import-of-global-command-object) default import of the global Command object, you need to switch to using a named import (or create a new `Command`).

```js
// const program = require('commander');
const { program } = require('commander');
```

**option and command clashes**

A couple of configuration problems now throw an error, which will pick up issues in existing programs:

-   adding an option which uses the same flag as a previous option
-   adding a command which uses the same name or alias as a previous command

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4wLjAiLCJ1cGRhdGVkSW5WZXIiOiIzNy4wLjAiLCJ0YXJnZXRCcmFuY2giOiJkZXZlbG9wIn0=-->

Reviewed-on: https://gitea.vylpes.xyz/RabbitLabs/random-bunny/pulls/145
Reviewed-by: Vylpes <ethan@vylpes.com>
Co-authored-by: Renovate Bot <renovate@vylpes.com>
Co-committed-by: Renovate Bot <renovate@vylpes.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@shadowspawn @aweebit