Conversation
| <> | ||
| <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> |
There was a problem hiding this comment.
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} />); |
| <WithinCommunityByline contributors={authors} linkToUsers={false} /> | ||
| </> | ||
| ); | ||
| return <WithinCommunityByline contributors={authors} linkToUsers={false} />; |
| <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> |
| <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} | ||
| /> |
| <> | ||
| <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 }); | ||
| }} | ||
| /> |
There was a problem hiding this comment.
fragment removed
ill stop marking these now
| 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]), |
There was a problem hiding this comment.
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
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!
| if (key === 'createdAt' || key === 'updatedAt') { | ||
| return key; | ||
| } |
There was a problem hiding this comment.
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.
| export const client = initClient(contract, { | ||
| baseUrl: '', | ||
| baseHeaders: {}, | ||
| credentials: 'include', | ||
| }); | ||
|
|
There was a problem hiding this comment.
i wasn't using this client for anything, kind of weird to have it here
3mcd
left a comment
There was a problem hiding this comment.
LGTM, other than the failing tests. Know what's causing that?
… by up to 30% uncompressed/17% compressed (#2921)
… undefined.pubpub.org (#2947)
…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>
#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
|
Sorry, totally missed that you had approved this @3mcd ! The only significant change is a type change, namely making the update params for facets all partial. I.e., before the types looked like and now the look like 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. |
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.
loggedInThis can be
true(default),falseor"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 theGETroutes.sinceThis is to mark since when this route has been available in the SDK. Not using this atm.
exampleHere you can write an example in Markdown of how this code should be used. We are using
tsdocspecification here, so@exampletags 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
to
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-restBasically, unless we do this AND fork
ts-restto export some key types, typescript will inline every single type in the outputd.tsfiles of the client, i.e. dereference every single type and just print the finalized object type. So noProxyClient<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 thisasnonsense herehttps://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
typedocto generate docs for the client.This was easier said then done, as
typedocREALLY 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
typedocto generate correct documentation for nested contracts.E.g. this lead to correct documentation
while this did not
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
GET /api/logoutPOST /api/logininto
GET /api/logoutPOST /api/loginI grouped
GET /api/workerTasksPOST /api/importPOST /api/exportinto
GET /api/workerTasksPOST /api/importPOST /api/exportI grouped
POST /api/uploadPOST /api/uploadPolicyinto
POST /api/uploadPOST /api/uploadPolicyThat'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
to
I want to do a similar thing with
pubAttributionsforpubs.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
prettierplugin,prettier-plugin-jsdoc, to make sure the JSDocs were formatted correctly.Unfortunately, this meant upgrading to
prettierv3, which meant upgrading theeslint-plugin-prettierextension, which meant that a whole lot of files were throwing errors duringnpm 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
prettierin a separate PR.