Skip to content

feat: add jsdoc documentation generation generation for ts-rest contracts#2937

Merged
tefkah merged 43 commits intomasterfrom
tfk/jsdoc
Jan 10, 2024
Merged

feat: add jsdoc documentation generation generation for ts-rest contracts#2937
tefkah merged 43 commits intomasterfrom
tfk/jsdoc

Conversation

@tefkah
Copy link
Copy Markdown
Member

@tefkah tefkah commented Dec 8, 2023

Issue(s) Resolved

#2869

Test Plan

Run tests

Notes/Context/Gotchas

This PR adds the ability to autogenerate JSDoc comments for the API routes defined by the ts-rest schemas.

Roughly, this here file
https://github.com/pubpub/pubpub/blob/61a53d5063009a1226de48dce445fc407997d0bf/tools/generateJSDoc.ts#L1-L115

defines a script that uses the typescript compiler API + ts-morph (a library that makes working with said compiler a bit easier) to look for the description, path, method, and metadata fields of a route, and generates a JSDoc comment that describes the route.

This JSDoc comment will then show up in the SDK as intellisense hints, and, by using typedoc, will be shown in the documentation for the SDK.

An example

This route
https://github.com/pubpub/pubpub/blob/84f83d2e16c6ded36206e6d5ec54d5516d090a8c/utils/api/contracts/customScript.ts#L32-L54

will receive such a comment

https://github.com/pubpub/pubpub/blob/84f83d2e16c6ded36206e6d5ec54d5516d090a8c/utils/api/contracts/customScript.ts#L10-L31

Metadata

For now there are three pieces of metadata you can specify.
This information can also be used by the OpenAPI docs, which I intend to add, but haven't yet. For instance you can mark whether a route needs cookie authentication this way, which currently is very vague.

loggedIn

This can be true (default), false or "admin".

  • true: you need to be logged in and have access to this resource in order to use this route. Most routes are like this.
  • false: you do not need to be logged in in order to use this route.
  • "admin": you need to be logged in as an admin of this community to use this route. These routes are the newer ones, such as upload, import, and most of the GET routes.

since

This is to mark since when this route has been available in the SDK. Not using this atm.

example

Here you can write an example in Markdown of how this code should be used. We are using tsdoc specification here, so @example tags are interpreted as Markdown, not code. If you want code, you need to manually specify a code block, see the example above.

Weird reordering

As you may have noticed, I shuffled around the contracts a bit.

They went from

// contracts/example.ts

import { initContract } from '@ts-rest/core'

const c = initContract()

export const exampleContract = c.router({
      get: {
          // route info
      }
})

to

// contracts/example.ts
import type { AppRouter } from '@ts-rest/core'

export const exampleRouter = {
      get: {
        // route info
     }
} as const satisfies AppRouter

type ExampleRouterType = typeof exampleRouter

export interface ExampleRouter extends ExampleRouterType {}

You may be wondering: why????

Well, dear reader, it's because typescript is an evil language designed to make me suffer. And because of implementation details in ts-rest

Basically, unless we do this AND fork ts-rest to export some key types, typescript will inline every single type in the output d.ts files of the client, i.e. dereference every single type and just print the finalized object type. So no ProxyClient<ApplyAppRouter<typeof exampleRouter>>, but { get: (input: { body: /* ... */ }) => { /*...*/}) }.

That alone would not be a big problem, sure the output file is a bit big but it's just types, who cares.

The problem is that typescript gets rid of the JSDoc comments when it does this. Which sucks as a user of the API client, as you miss a lot of context.

I can go into why this works a bit more elsewhere, but tl;dr: to prevent typescript from dereferencing/inlining types we force two important types to be exported by ts-rest (otherwise, in the output d.ts, typescript is forced to inline it as it cannot reference them obviously), and force these interfaces to be used as the type by doing this as nonsense here

https://github.com/pubpub/pubpub/blob/ed5bbcfcfc4d1e7119ec9ecb72b231c25b594ea7/utils/api/contract.ts#L27-L45

Typescript doesn't like to dereference interfaces nearly as much as types.

Because of all this, typescript keeps references to the original route in the d.ts, which has the JSDoc annotations, which means that consumers of the sdk will have annotations, woohoo!

Shifting around things for documentation purposes

The other goal of the JSDoc comments was to use typedoc to generate docs for the client.

This was easier said then done, as typedoc REALLY didn't want to properly show certain types, but after some (a lot) of coercing i managed to find some things that work, see the documentation about the documentation at https://github.com/pubpub/sdk.

But tl;dr, I could only manage typedoc to generate correct documentation for nested contracts.

E.g. this lead to correct documentation

export const contract = c.router({
      pub: {
         get: {
           // route
        }
     }
})

while this did not

export const contract = c.router({
      getPub: {
          // route
     }
})

Why? I don't know. I'm sure it has something to do with how I mangle the types for the client, but this is the only way I managed to get it to work. Seeing how much time I already wasted trying to generate the docs nicely, I found reorganizing some floating routes to be a worthwhile sacrifice.

These groupings only affect the API of the SDK and how certain routes are written, not any implementation or path changes. This will also change how the OpenAPI spec is displayed, as these grouping represent these tabs on the left side.

Reorganization details

I grouped

  • logout: GET /api/logout
  • login: POST /api/login

into

  • auth
    • logout: GET /api/logout
    • login: POST /api/login

I grouped

  • workerTask
    • get: GET /api/workerTasks
  • import: POST /api/import
  • export: POST /api/export

into

  • workerTask
    • get: GET /api/workerTasks
    • createImport: POST /api/import
    • createExport: POST /api/export

I grouped

  • upload: POST /api/upload
  • uploadPolicy: POST /api/uploadPolicy

into

  • upload
    • file: POST /api/upload
    • uploadPolicy: POST /api/uploadPolicy

That's it!

Ideas for further grouping

Doing this made me realize: I could have done this all along!

I have not implemented this yet, but I would like to propose the following groupings

  • collection
    • some routes
  • collectionAttribution
    • get
    • getMany
    • create
    • remove
  • collectionPub
    • get
    • create
    • remove

to

  • collection
    • attribution
      • get
      • getMany
      • create
      • remove
    • pub
      • get
      • add <-- i think this is clearer, you are adding a pub to a collection
      • remove

I want to do a similar thing with pubAttributions for pubs.

I think this is much clearer.

The only downside is that we lose the http path significance, but I tihnk it's worth it.

Prettier...

As you may have seen from the gigantic amount of files changed, something happened.
That something was that I wanted to use a certain prettier plugin, prettier-plugin-jsdoc, to make sure the JSDocs were formatted correctly.

Unfortunately, this meant upgrading to prettier v3, which meant upgrading the eslint-plugin-prettier extension, which meant that a whole lot of files were throwing errors during npm run lint.
I fixed these errors, but that came with the downside that now a lot of files have been changed.

This is super cringe, but I think an okay sacrifice. I think we would have to upgrade prettier anyway, might as well be now!

The PR is very messy bc of this though, apologies! I can revert it if you find it important, and upgrade prettier in a separate PR.

Comment on lines -18 to +20
<>
<React.Fragment>
<Tooltip content={copy}>
<span className={Classes.TOOLTIP_INDICATOR}>{children}</span>
</Tooltip>
</React.Fragment>
</>
<Tooltip content={copy}>
<span className={Classes.TOOLTIP_INDICATOR}>{children}</span>
</Tooltip>
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

eslint seems to have changed this. (i ran eslint --fix to fix all the prettier changes, this may have been caught in that)

I think this shouldn't do anything, unless you were somehow relying on some evil react thingy

<PubEdge pubEdge={pubEdgeWithoutAvatar} />
</>
));
.add('no avatar', () => <PubEdge pubEdge={pubEdgeWithoutAvatar} />);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

same here

<WithinCommunityByline contributors={authors} linkToUsers={false} />
</>
);
return <WithinCommunityByline contributors={authors} linkToUsers={false} />;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fragment removed

Comment on lines -418 to +421
<React.Fragment>
<div className="testimonial" key={example.source}>
<div className="testimonial-quote">{example.quote}</div>
<div className="testimonial-source">
— {example.source}
</div>
</div>
</React.Fragment>
<div className="testimonial" key={example.source}>
<div className="testimonial-quote">{example.quote}</div>
<div className="testimonial-source">— {example.source}</div>
</div>
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fragment removed

Comment on lines -71 to +78
<React.Fragment>
<Button
type="button"
intent="primary"
text="Save Changes"
disabled={!canPersistChanges}
loading={isPersisting}
onClick={handleSaveChanges}
/>
</React.Fragment>
<Button
type="button"
intent="primary"
text="Save Changes"
disabled={!canPersistChanges}
loading={isPersisting}
onClick={handleSaveChanges}
/>
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fragment removed

Comment on lines -30 to +42
<>
<NavBuilder
initialNav={communityData.navigation ?? []}
prefix={[
...(communityData.navigation?.[0]
? [communityData.navigation[0]]
: ([] as CommunityNavigationEntry[])),
]}
pages={pages}
collections={collections}
onChange={(val) => {
updateCommunityData({ navigation: val });
}}
/>
</>
<NavBuilder
initialNav={communityData.navigation ?? []}
prefix={[
...(communityData.navigation?.[0]
? [communityData.navigation[0]]
: ([] as CommunityNavigationEntry[])),
]}
pages={pages}
collections={collections}
onChange={(val) => {
updateCommunityData({ navigation: val });
}}
/>
Copy link
Copy Markdown
Member Author

@tefkah tefkah Dec 11, 2023

Choose a reason for hiding this comment

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

fragment removed
ill stop marking these now

Comment thread utils/api/schemas/pub.ts
Comment on lines -249 to +254
files: z.union([
z.array(
z
.tuple([z.custom<Blob>(), z.string({ description: 'filename' })])
.openapi({ title: 'Blob + filename (Node 18)' }),
),
z.custom<File[]>().openapi({ title: 'File (Node 20+, browser)' }),
]),
files: z.union([z.array(fileSchema), fileSchema]),
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this is kind of a significant change, and is related to something i changed in our fork of ts-rest, which i'll explain here.

ts-rest does not do arrays well. if it encounters an array in a field in an object that gets converted to formdata, it just appends the whole array, which often leads you with "['object Object', 'object Object']" as the field, which is silly.

before, I would allow users to pass either a File (which is not available in node 18), or [Blob, string]. I would then manually, in the sdk, create methods that merge these things and do something like

upload({ files }) {
  const form = new FormData()
  if(Array.isArray(files)){
       files.forEach(([blob, filename]) => {
          form.append('files', blob, filename)
       })
  }
// ...
}

which makes formdata sort of manually create a file. Kind of weird but whatever.

I decided to get rid of those manual methods in the sdk, because, as we were forking ts-rest anyway, might as well have it do what we want it to do.

See this diff for what i changed

pubpub/ts-rest@b533bdd...fe7c011#diff-aa23f3f5a1b6dab4c323411e117b8624de01d63a532d78e5138363cde570eba3

Long story short, allowing for both File and a Blob/filename pair, and arrays of those, is way cleaner if the Blob/filename pair is an object.

so to upload files, you can now do

await pubpub.upload.file({
     files: [{ blob: new Blob(['hey'], 'text/plain'), filename: 'hey.txt' }]
})

Neat!

Comment on lines +11 to +13
if (key === 'createdAt' || key === 'updatedAt') {
return key;
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

to create the filter query param schema, i filter out all optional() fields, which are normally associations and I don't want to allow filtering by association (e.g. show me all collections with a Pub with name 'My Pub', too complex).
However, due to annoying reasons, I needed to make the createdAt and updatedAt optional as well, so I allow an exception for them.

Comment thread utils/api/client.ts
Comment on lines -4 to -9
export const client = initClient(contract, {
baseUrl: '',
baseHeaders: {},
credentials: 'include',
});

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

i wasn't using this client for anything, kind of weird to have it here

Copy link
Copy Markdown
Collaborator

@3mcd 3mcd left a comment

Choose a reason for hiding this comment

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

LGTM, other than the failing tests. Know what's causing that?

@pubpubBot pubpubBot temporarily deployed to pubpub-pipel-tfk-jsdoc-6q8oywk December 12, 2023 17:01 Inactive
tefkah and others added 15 commits December 12, 2023 18:21
…ld 500 (#2948)

* fix: include community in pub edge includes in for sibling edges to prevent weird connection 500 edge case

* fix: revert base hostname for pubdoc to be req.hostname as that has been proven to work

* fix: type error

* check for domain in duqduq as well

* revert

---------

Co-authored-by: Gabriel Stein <g@gabestein.com>
…age size by up to 30% uncompressed/17% compressed (#2921)" (#2949)

This reverts commit 434aceb.
#2952)

* fix: prevent api route not returning by wrapping actually throwing in /api/citations/zotero

* fix: restore await

* fix: add test to prevent regression
…tier (#2955)

* fix: show better error message for already used slugs for pubs

* fix: add tests and more readable zod errors

* fix: actually return the validation error message
* feat: purge session cache on logout

* fix: add test
…ties (#2960)

* fix: new caching for logged in users and fixes for user pages

* fix: bust user cache on notification

* fix: purge user pages after user is added or deleted as a member somewhere

* fix: its only pubattributions, not collections

* fix: modify tests

* fix: eslint

* feat: much better tests
#2963)

* feat(hooks.ts): add release hooks to create purge hooks for PubAttribution and Release models
fix(hooks.ts): remove unused imports and deferred function for purging user pages on PubAttribution creation
fix(hooks.ts): remove unused imports and deferred function for purging user pages on PubAttribution creation
fix(hooks.ts): remove unused imports and deferred function for purging user pages on PubAttribution creation
fix(user.tsx): change setSurrogateKeys function to include user slug and request hostname in Surrogate-Key header
test(purge.test.ts): add test case for purging user pages on Release creation
feat(createPurgeHooks.ts): create utility function to generate purge hooks for models and defer purging logic
fix(purgeMiddleware.ts): change req.user.id to req.user.slug for purging user pages

* refactor: move user update listen to new purgehooks api
* add biophysics colab

* add slightly bigger header as example
@pubpubBot pubpubBot temporarily deployed to pubpub-pipel-tfk-jsdoc-auuut2d January 10, 2024 11:57 Inactive
@tefkah tefkah temporarily deployed to pubpub-pipel-tfk-jsdoc-auuut2d January 10, 2024 12:09 Inactive
@tefkah
Copy link
Copy Markdown
Member Author

tefkah commented Jan 10, 2024

Sorry, totally missed that you had approved this @3mcd !
Made some tiny adjustments that fix some small issues on the SDK, and merged this with master.

The only significant change is a type change, namely making the update params for facets all partial.

I.e., before the types looked like

{
  facets?: {
	PubHeaderTheme?: {
		backgroundImage: string
		backgroundColor: string
		theme: string
    }
    ....
}

and now the look like

{
  facets?: {
	PubHeaderTheme?: {
		backgroundImage?: string
		backgroundColor?: string
		theme?: string
    }
    ....
}

This reflect more closely what the api actually accepts, and allows you to update facets without having to specifiy every single part of that facet, just the one you need.

i.e. i can now change the background color by doing

pubpub.facets.update({
   PubHeaderTheme: {
		backgroundColor: '#000000'
	}
})

instead of also having to specify the background image and theme.

This was already how the API worked, the types now more accurately reflect this.

@tefkah tefkah merged commit 2dfe92a into master Jan 10, 2024
@tefkah tefkah deleted the tfk/jsdoc branch January 10, 2024 13:30
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