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

Fix documentation rendering #965

Open
wants to merge 65 commits into
base: master
Choose a base branch
from

Conversation

ElliotFriend
Copy link

I'm trying to get all the JSDoc rendering to work now that the SDK has had a chance to settle a bit with its inclusion of the Soroban functionality.

Refs: #920

Copy link

github-actions bot commented May 14, 2024

Size Change: +74.8 kB (+0.61%)

Total Size: 12.3 MB

Filename Size Change
dist/stellar-sdk.js 6.89 MB +48.3 kB (+0.71%)
dist/stellar-sdk.min.js 5.41 MB +26.5 kB (+0.49%)

compressed-size-action

@Shaptic
Copy link
Contributor

Shaptic commented May 14, 2024

Yo this is absolutely Herculean, you freakin' rock

Since the comments clearly say "don't make this class yourself,"
it might make sense to mark them as `@private` so they don't show
up in the documentation.

The doc pages for those classes can still be visited, but they
don't show up in the sidebar to the left, unless you specifically
navigate to one of them. (Or, if you click a link for what the
return type on the `Horizon.Server.effects()` method, for example
I don't _think_ they're accessible by SDK users, so I think it
makes sense to not render them in the docs?
@Shaptic Shaptic mentioned this pull request Jun 18, 2024
@ElliotFriend
Copy link
Author

@Shaptic I think this is ready to go! Thanks for checking it out!

@ElliotFriend ElliotFriend marked this pull request as ready for review September 20, 2024 17:50
Copy link
Contributor

@Shaptic Shaptic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rocks my socks off 🎸 AMAZING work 👏

Can you make sure you run yarn _prettier one more time? Then I'll merge

* @property {string} [authToken] Allow set custom header `X-Auth-Token`, default: `undefined`.
*/

export namespace HorizonServer {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has no tangible side-effects to downstream users because of the aliasing in index.ts, right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in my local testing, it behaves like normal.

  • I tested the /lib output by using node in my terminal window, and interacting there.
  • I tested the /dist output by creating a dummy html page that added the /dist/stellar-sdk.js file in a <script> tag and interacting with StellarSdk in the browser console.

Those may not be the best ways to test, though, so if there's another method, I'm certainly down for that.

@ElliotFriend
Copy link
Author

@Shaptic when I run yarn _prettier, there are no resulting changes. Which seems... unlikely lol. If i'm reading the underlying command right, it seems like it's actually only running on the /tests directory?

"_prettier": "prettier --ignore-path config/.prettierignore --write './test/**/*.js'"

If I run that replacing ./test/**/*.js with ./src/**/*.ts, there's plenty of fixes to go around, though lol. Want me to commit that?

@ElliotFriend
Copy link
Author

ElliotFriend commented Sep 20, 2024

I ended up just committing all those changes in a single commit, so it'll be easy enough to revert if you want to: 841f7c9

Edit: I did a bit of digging, and it looks like commit 21bd09 from #860 is when the prettier directory changed from ./**/*.js to ./tests/**/*.js. Don't know why the change was made, but at least i found it 🤣

Another Edit: I built and previewed the docs after the prettier changes, and everything looks the same as before. Still good to go, I think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants