-
Notifications
You must be signed in to change notification settings - Fork 512
docs: Check TS in docs using typescript-docs-verifier #1505
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
Conversation
f419815 to
894ee06
Compare
achingbrain
left a comment
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 great, really helpful.
I think this should be added to aegir as a doc-check command though, similar to dep-check.
It can manage the tsconfig.json file for the docs then, in the same way we do it for linting.
It also doesn't currently check any code blocks in the docs here, because they are fenced with js as a type and not typescript - ts doesn't seem to work either, though maybe there's some config for that?
|
Thanks for the feedback.
Good point, I'll add it as a feature
A user may want to use a different
I think it's because of you were traversing the wrong directory in the script here it works otherwise. I opened a ticket on aegir once this is closed we can update and review this. |
|
Once ipfs/aegir#1134 is merged and released then this can be reviewed |
dba4a95 to
453857d
Compare
453857d to
a351954
Compare
achingbrain
left a comment
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.
Please can you revert all the formatting changes and stick to the same coding standards as the main codebase. Single quotes for strings, no semi colons, no extra comma for the final entry in a list, etc.
|
The interop suite has a peer dep on [email protected] so the dependency tree can't be resolved: This is blocked until libp2p/interop#82 is merged. |
|
TBH we may wish to do the aegir upgrade first in it's own PR, then revisit this one once it's merged as adding return types to all the functions will be quite noisy. |
|
I've upgraded to the |
|
this add deps on prom-metrics in the libp2p repo, will wait for the monorepo work to get in follow up in #824 |
achingbrain
left a comment
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.
The /doc folder should be added to the workspaces list in the root package.json.
It should have a package.json added that contains:
- The
doc-checkcommand - The deps needed by the doc examples
- The
dep-checkcommand to ensure the deps don't get out of date (assuming we can make it run on markdown, might need to be a follow up)
| ], | ||
| }, |
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.
These comments should follow the same linting rules as the main codebase; comma-dangle is disallowed.
| ], | |
| }, | |
| ] | |
| } |
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.
So the doc-check command which utilizes https://github.com/bbc/typescript-docs-verifier only checks that Typescript code snippets compile, it's not a linter. That could be added in a follow up PR but wasn't the original intention of this.
|
I've added the
Both of these will be features to be added in https://github.com/ipfs/aegir |
achingbrain
left a comment
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.
LGTM, just needs the private flag added to the docs package.json so it doesn't get published.
Co-authored-by: Alex Potsides <[email protected]>
Closes #1228