paste icon indicating copy to clipboard operation
paste copied to clipboard

Adding JSDoc to Paste Components

Open cogwizzle opened this issue 9 months ago • 8 comments

The purpose of this Pull Request is to add some JSDoc to the Account Switcher, Alert, and Alert Dialog components.

There are a few minor refactors that accompany this Pull Request that should not impact the functionality of the component. These changes are purely focused on reducing boilerplate or reducing the complexity of the syntax of the components. All of these changes are confined in the AccountSwitcher, Alert, or Alert Dialog components or their supporting components.

Additional JSDoc changes will be coming however, new changes will be broken out into individual component Pull Request to reduce the load on the reviewing engineer.

Contributing to Twilio

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

  • [X] I acknowledge that all my contributions will be made under the project's license.

cogwizzle avatar May 10 '24 17:05 cogwizzle

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

stackblitz[bot] avatar May 10 '24 17:05 stackblitz[bot]

⚠️ No Changeset found

Latest commit: c8fc1c13d0d474513f084ec437be82a2c5650712

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

changeset-bot[bot] avatar May 10 '24 17:05 changeset-bot[bot]

@cogwizzle is attempting to deploy a commit to the Twilio Team on Vercel.

A member of the Team first needs to authorize it.

vercel[bot] avatar May 10 '24 17:05 vercel[bot]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit c8fc1c13d0d474513f084ec437be82a2c5650712. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

nx-cloud[bot] avatar May 10 '24 17:05 nx-cloud[bot]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit c8fc1c13d0d474513f084ec437be82a2c5650712:

Sandbox Source
@twilio-paste/nextjs-template Configuration
@twilio-paste/token-contrast-checker Configuration

codesandbox-ci[bot] avatar May 10 '24 17:05 codesandbox-ci[bot]

Hey @cogwizzle - thanks for this contribution!

I'd like to think a bit more about these changes. I definitely see the value in having the JSDoc comments on the consumer-facing components that include the package description and the documentation link, but I'm a little concerned about the maintenance there. It's not the most common, but sometimes we do change package descriptions, and there are already multiple places we need to maintain them (the package's package.json and all 3 files within the packages/paste-website/src/pages/components/* directory). Having an additional place in the component file itself seems like too much upkeep that's prone to human error. I wonder if we could instead pull the description from the package.json to create the comments. It would be helpful to similarly import or dynamically create the documentation url so that doesn't add additional labor to the creation of new components, but I'd have to do more thinking as to whether that's possible as some of the docs urls aren't exactly standard (e.g. Sidebar).

In terms of the non-consumer-facing components, could you explain what the value-add is for adding JSDoc comments? Such as AlertDialogBody, AlertDialogFooterProps props, AlertIcon, etc. If it's for contributors, I'm not completely convinced it outweighs the clutter of having them in the docs and effort of creating and maintaining them. I'm also not totally convinced that consumer-facing components that exist within a single component (e.g. Account Switcher's AccountSwitcherBadge component) need to have the comments. It should be clear from the documentation what these compositional components are for and creating descriptions for each one for the comments will similarly take additional time and effort. Happy to be swayed otherwise (on any of this).

If we decide to move forward with these JSDoc comments (or some portion of them, even if only on consumer-facing components), it would be great to get them all out across the board in a relatively timely manner, so that the change happens across not too many upgrades for consumers. In the past when we've done core-wide changes, we've split them up alphabetically into batches (a-c, d-h, i-p, etc) so just sharing that as an option.

Either way, thanks again for this improvement and for all of your recent improvements. It's great to have consumers of Paste contributing and you bring a really valuable perspective to the project! Sadly the team is operating at extremely low capacity right now so we can't get them reviewed and merged as quickly as we'd like, but we appreciate your patience.

Thanks so much for reviewing this PR. I agree that the maintenance upkeep might too high. I would love to find a way to pull the documentation from an external source or to find a way to keep them in sync.

As far as the value proposition it is extremely helpful to understand the use case for a component while using a library. JSDoc is just the way that I am aware of to make this happen. Perhaps there is another way to get the Language Server to add context. A good example of where a consumer of paste would need context is for things like button accessibility guidance. Without this great guidance an engineer could consume this in the wrong way. I doubt every engineer hits the Paste site every time they want to consume a component. It is unlikely the consumer of the library would see this great advice if they did not have the JSDoc that we previously added for this. There are also other contextually important information such as understanding when to use an Alert vs when to use a Callout. The description information in the documentation can help here.

This contextual information is relevant when consumers use libraries and they can use the TypeScript language server to give details about the element. Another tangental thing we could do with JSDoc is use it for typing if we wanted to drop TypeScript, but that doesn't seem relevant in this project.

Here is an example of storybook providing its users contextual information in TS Server. I'm not sure if they're using JSDoc or some other method to accomplish this. image

The short version is I want all of the great guidance that exist in the Paste docs to be quickly accessible from the consuming engineers.

PS: For the non public stuff I just added JSDoc for consistency. I wasn't 100% sure that they wouldn't be used outside of the Paste project.

cogwizzle avatar May 24 '24 14:05 cogwizzle

I wonder if we could take the opposite approach to solving the TSDoc problem. I wonder if we could extract a basic description from the code files and then have elaborative details. This is a tough problem to solve either way.

cogwizzle avatar May 24 '24 14:05 cogwizzle

I wonder if we could take the opposite approach to solving the TSDoc problem. I wonder if we could extract a basic description from the code files and then have elaborative details. This is a tough problem to solve either way.

Apparently it is possible https://tsdoc.org/#why-do-we-need-tsdoc. Perhaps it is worth digging into?

cogwizzle avatar May 24 '24 14:05 cogwizzle