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

[...] Start JSDoc type linting #6770

Merged
merged 12 commits into from
Nov 2, 2019

Conversation

brody4hire
Copy link

@brody4hire brody4hire commented Oct 31, 2019

As requested by @thorn0 in PR #6709, here are some updates to start JSDoc type linting using the TypeScript compiler (tsc) as described by @rbiggs in: https://medium.com/@trukrs/javascript-type-linting-5903e9e3625f

The idea is that we can enable JSDoc type linting on more and more sources as we are able to resolve the JSDoc type errors. This would be a partial solution to #6703, as an alternative to #6286 (convert to TypeScript).

I am using the tsconfig.json file from PR #6313 by @Shinigami92, with the following updates:

  • add more entries to excludes to exclude the src files that do not yet pass JSDoc type linting
  • minor updates to settings
  • add checkjs script and integrate it with the Dev_Lint task on Azure Pipelines
  • introduce JSDoc type linting with strict mode enabled in tsconfig-strict.json, on a limited number of src files for now, and add it to checkjs script
  • resolve JSDoc type error in src/doc/doc-printer.js, as I proposed in PR fix JSDoc type error in src/doc/doc-printer.js #6709
  • add JSDoc type improvements to src/doc/doc-builders.js, as I proposed in PR [RFC DRAFT WIP] add & apply doc-builders.js type improvements #6710
  • other fixes & updates, in response to comments by @thorn0

In case we do not want strict JSDoc type checking, we can just remove use of tsconfig-strict.json. I do think it would be ideal to use strict type checking as much as possible.

There is now a limited number of JSDoc improvements and fixes within this PR. I think further updates and fixes should be done in other PRs, assuming this proposal is accepted and merged.

The changes in this proposal includes work by multiple authors. At this point the last commit as Co-Authored-By comments which should hopefully preserve the proper co-authorship credits. I would like to ask that the Co-Authored-By comments be preserved if this proposal is accepted and merged.

/cc @Shinigami92 @thorn0 @evilebottnawi @rbiggs

TODO items are now tracked in TBD comments.

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • (If the change is user-facing) I’ve added my changes to the CHANGELOG.unreleased.md file following the template.
  • I’ve read the contributing guidelines.

Try the playground for this PR

Christopher Quadflieg and others added 3 commits July 18, 2019 16:51
list in tsconfig.json

with [TBD] comments

at this point, `npx tsc` should not show any JSDoc type issues
package.json Outdated Show resolved Hide resolved
tsconfig-strict.json Outdated Show resolved Hide resolved
src/doc/doc-builders.js Outdated Show resolved Hide resolved
src/doc/doc-builders.js Outdated Show resolved Hide resolved
jest.config.js Outdated Show resolved Hide resolved
src/doc/doc-builders.js Outdated Show resolved Hide resolved
@brody4hire
Copy link
Author

I think the changes should be almost ready to go. All "questionable" JSDoc comment annotations now have "TBD" comments to hopefully ensure they are not forgotten in the future.

I hope we can get this approved and merged soon to avoid blocking further JSDoc type fixes.

My one question is whether we should expect a "squash merge", a merge commit, or something else?

I would like to rebase and cleanup the commit messages before we go through the final review and merge.

Any further questions or concerns would be appreciated as well. Thanks!

@thorn0
Copy link
Member

thorn0 commented Nov 1, 2019

We always squash merge in this project.

Christopher J. Brody added 5 commits October 31, 2019 20:29
which simply invokes `tsc` for JSDoc type checking
as specified in `tsconfig.json`
as proposed in PR prettier#6709

no longer excluded in tsconfig.json

also with `src/doc/index.js` no longer excluded in tsconfig.json
to pass strict JSDoc type linting

as already proposed in PR prettier#6710

with `src/doc/doc-builders.js` now included in `tsconfig-strict.json`
@brody4hire brody4hire force-pushed the start-jsdoc-type-linting branch from 55df362 to 43393f4 Compare November 1, 2019 00:45
src/doc/doc-printer.js Outdated Show resolved Hide resolved
Chris Brody and others added 3 commits October 31, 2019 21:25
…s.js`

as detected by using `eslint-plugin-jsdoc` (locally)
as recommended by using `eslint-plugin-jsdoc` (locally)
@thorn0
Copy link
Member

thorn0 commented Nov 1, 2019

In general, you don't need to add @returns. TS infers it.

@brody4hire
Copy link
Author

In general, you don't need to add @returns. TS infers it.

Thanks for the info, really interesting.

When I tried using eslint-plugin-jsdoc in my local workarea, it suggested I was missing the @returns.

I would personally favor making it more explicit if it does not make things too messy. I tend to prefer to keep this kind of thing more explicit in JavaDoc & JSDoc, even if it is just documentation internal to the source module itself. Please let me know if you can think of a really compelling reason to take it back out.

Co-Authored-By: Christopher J. Brody <[email protected]>
Co-Authored-By: Christopher Quadflieg <[email protected]>
Co-Authored-By: Georgii Dolzhykov <[email protected]>
@brody4hire brody4hire changed the title [RFC ...] Start JSDoc type linting [...] Start JSDoc type linting Nov 1, 2019
@brody4hire brody4hire marked this pull request as ready for review November 1, 2019 02:33
@lipis
Copy link
Member

lipis commented Nov 1, 2019

If we switch to (actually when we switch) TS these JSDocs won't be needed.. right?

@brody4hire
Copy link
Author

If we switch to (actually when we switch) TS these JSDocs won't be needed.. right?

My understanding of the discussion in #6286 is that it is not desired to convert this project to TypeScript. I personally agree with this position very strongly.

Even if someone would convert this project to TypeScript, I would think that the JSDoc comments would still be nice internal documentation, in a similar fashion to how people can use JavaDoc comments to document Java code both internally and externally.

Am I missing something here?

@rbiggs
Copy link

rbiggs commented Nov 1, 2019

Well, good TS always has TSDoc comments, which is just a superset of JSDoc. This provides richer intellisense. Just look at the d.ts files that the TypeScript install provides: https://github.com/microsoft/TypeScript/blob/master/lib/lib.es2015.core.d.ts

It would be trivial to convert good JSDoc comments to TSDoc comments.

@lipis
Copy link
Member

lipis commented Nov 1, 2019

It would be trivial to convert good JSDoc comments to TSDoc comments.

Fair enough!

As for converting this project to TypeScript, I think sooner or later every JS project should be converted to TS, and Prettier is not an exception.

@rbiggs
Copy link

rbiggs commented Nov 1, 2019

To be honest, I don't think so. 10 years ago everyone had to know how to use JQuery or you weren't considered a good developer. Then came CoffeeScript, again everyone had to write CoffeeScript because it gave you error-free JavaScript. Now the new thing is TypeScript. 10-15 years from now no one will care about TypeScript because JavaScript will have its own type system that works at runtime. FYI JavaScript 4.0 was supposed to have that. At the time Microsoft apposed implementing typed JavaScript because they didn't want to have to re-write IE6. Instead everyone settled on JavaScript 3.5, which as not much compared to what the proposals were for JS4.0.

@thorn0
Copy link
Member

thorn0 commented Nov 1, 2019

@brodybits re @returns

Please let me know if you can think of a really compelling reason to take it back out.

I don't feel strongly about it. They are useful mainly for long functions with multiple return statements to control that the function doesn't return something unintended.

@rbiggs Off topic, but it was called ES 3.1, not 3.5. The whole story can be read here. Also TSDoc is rather a subset of JSDoc because it's the same thing minus types.

@lydell lydell merged commit e50ad94 into prettier:master Nov 2, 2019
@Shinigami92
Copy link
Member

🎉

@orta
Copy link
Contributor

orta commented Nov 14, 2019

This is super cool!

@thorn0
Copy link
Member

thorn0 commented Nov 15, 2019

@orta It probably is, but TS's type system seems to be still not flexible enough for our use cases. See microsoft/TypeScript#35045 (comment)

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Feb 13, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants