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

docs: Check TS in docs using typescript-docs-verifier #1505

Merged
merged 21 commits into from
Jul 4, 2023

Conversation

maschad
Copy link
Member

@maschad maschad commented Dec 2, 2022

Closes #1228

Copy link
Member

@achingbrain achingbrain left a 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?

doc/tsconfig.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
@p-shahi p-shahi added this to the Best Effort Track milestone Dec 6, 2022
@maschad
Copy link
Member Author

maschad commented Dec 6, 2022

Thanks for the feedback.

I think this should be added to aegir as a doc-check command though, similar to dep-check.

Good point, I'll add it as a feature

It can manage the tsconfig.json file for the docs then, in the same way we do it for linting.

A user may want to use a different tsconfig.json in situations like this for instance, so it may be best to specify that separately.

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?

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.

@maschad
Copy link
Member Author

maschad commented Dec 19, 2022

Once ipfs/aegir#1134 is merged and released then this can be reviewed

@maschad maschad force-pushed the docs/check-ts-readme branch 2 times, most recently from dba4a95 to 453857d Compare January 13, 2023 00:44
doc/LIMITS.md Outdated Show resolved Hide resolved
doc/LIMITS.md Outdated Show resolved Hide resolved
Copy link
Member

@achingbrain achingbrain left a 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.

doc/LIMITS.md Outdated Show resolved Hide resolved
@achingbrain
Copy link
Member

The interop suite has a peer dep on aegir@37.x so the dependency tree can't be resolved:

npm ERR! Could not resolve dependency:
npm ERR! peer aegir@"^37.2.0" from @libp2p/interop@4.0.2
npm ERR! node_modules/@libp2p/interop
npm ERR!   dev @libp2p/interop@"^4.0.0" from the root project

This is blocked until libp2p/interop#82 is merged.

@achingbrain
Copy link
Member

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.

@maschad maschad mentioned this pull request Jan 20, 2023
maschad added a commit to maschad/js-libp2p that referenced this pull request Mar 3, 2023
maschad added a commit to maschad/js-libp2p that referenced this pull request Mar 5, 2023
maschad added a commit to maschad/js-libp2p that referenced this pull request Mar 5, 2023
@maschad maschad marked this pull request as ready for review April 21, 2023 18:14
@maschad maschad requested a review from p-shahi April 25, 2023 17:31
@maschad
Copy link
Member Author

maschad commented May 15, 2023

Once ipfs/aegir#1255 is merged then we can update to the newest aegir and then land this.

I've upgraded to the 39.0.9

achingbrain pushed a commit to ipfs/aegir that referenced this pull request May 26, 2023
@maschad maschad requested a review from achingbrain May 29, 2023 17:20
@p-shahi
Copy link
Member

p-shahi commented Jun 6, 2023

this add deps on prom-metrics in the libp2p repo, will wait for the monorepo work to get in follow up in #824
have package.json in docs folder to keep it out of libp2p deps

@p-shahi p-shahi added status/blocked Unable to be worked further until needs are met and removed status/blocked Unable to be worked further until needs are met labels Jun 14, 2023
@p-shahi
Copy link
Member

p-shahi commented Jun 14, 2023

this add deps on prom-metrics in the libp2p repo, will wait for the monorepo work to get in follow up in #824 have package.json in docs folder to keep it out of libp2p deps

Unblocked, @maschad adding to this PR itself

Copy link
Member

@achingbrain achingbrain left a 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:

  1. The doc-check command
  2. The deps needed by the doc examples
  3. The dep-check command 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)

Comment on lines +287 to +288
],
},
Copy link
Member

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.

Suggested change
],
},
]
}

Copy link
Member Author

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.

doc/LIMITS.md Show resolved Hide resolved
packages/libp2p/package.json Outdated Show resolved Hide resolved
packages/libp2p/package.json Outdated Show resolved Hide resolved
packages/libp2p/package.json Outdated Show resolved Hide resolved
packages/libp2p/package.json Outdated Show resolved Hide resolved
@maschad maschad requested a review from achingbrain June 28, 2023 21:48
@maschad
Copy link
Member Author

maschad commented Jun 28, 2023

I've added the doc-check command to the docs/ workspace. We will need a follow up PR for:

  • doing a dep-check than checks the TS snippets in markdown
  • Enforcing linting rules on TS snippets in markdown.

Both of these will be features to be added in https://github.com/ipfs/aegir

doc/package.json Outdated Show resolved Hide resolved
Copy link
Member

@achingbrain achingbrain left a 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 <alex@achingbrain.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Configuration page needs updates
3 participants