External API GET threads and topics #9102
Conversation
kurtisassad
force-pushed
the
ka.externalGetThreads
branch
from
25cda4c
to
052ee32
Compare
kurtisassad
changed the title
# Conflicts: # packages/commonwealth/server/api/external-router.ts
kurtisassad
force-pushed
the
ka.externalGetThreads
branch
from
b0e20cd
to
cd20621
Compare
| }), | ||
| }; | ||
|
|
||
| export const DEPRECATED_GetThreads = z.object({ |
There was a problem hiding this comment.
These schemas conflict with our naming convention but are used for the old API. I have prefixed them with DEPRECATED for now since I am unsure how to handle it.
# Conflicts: # libs/adapters/src/trpc/handlers.ts
kurtisassad
force-pushed
the
ka.externalGetThreads
branch
from
bcf9237
to
8b17eaa
Compare
There was a problem hiding this comment.
Generally looks good.
I tested setting all include options to false and it still returned the Address and User.
Are these routes new external routes duplicates of existing legacy routes used on the client? If so we should quickly follow-up with transitioning the legacy route usage to the new routes to avoid having to maintain both routes.
| include: includeArray, | ||
| ...formatSequelizePagination(payload), | ||
| paranoid: false, | ||
| } as unknown as FindAndCountOptions<ThreadAttributes>); |
There was a problem hiding this comment.
We can remove this type coercion from all future usage of formatSequelizePagination if we add a generic to the function which takes the attributes of the model and coerces order_by to one of the attributes instead of string.
There was a problem hiding this comment.
So it looks like the order_by is just typed as Order in sequelize, it does not depend on the key of the model. So I removed this as unknown typing, but just replaced it with the Order type without any generics
# Conflicts: # libs/model/src/thread/index.ts # packages/commonwealth/server/api/external-router.ts # packages/commonwealth/server/api/thread.ts
Fixed |
Yes they are new routes with our consistent parameter convention. We discussed this during platform chat a while back, and the conclusion was, we will expect frontend to implement them when they have time :) Then we will remove the old routes. |
Do we have a ticket for using the new routes introduced here on the front-end? Having 2 routes that do the same thing is super risky - can easily make a change on one side without realizing there is another route that needs the same change so this shouldn't be something that Peng just eventually gets to 3 months down the line. |
# Conflicts: # packages/commonwealth/server/api/internal-router.ts
# Conflicts: # libs/model/src/thread/index.ts # packages/commonwealth/server/api/external-router.ts # packages/commonwealth/server/routes/feed.ts
kurtisassad
force-pushed
the
ka.externalGetThreads
branch
from
42ff96c
to
1811800
Compare
# Conflicts: # packages/commonwealth/client/scripts/models/Topic.ts # packages/commonwealth/client/scripts/views/pages/discussions/HeaderWithFilters/HeaderWithFilters.tsx # packages/commonwealth/server/api/external-router.ts # packages/commonwealth/server/api/thread.ts
# Conflicts: # packages/commonwealth/client/scripts/views/components/NewThreadForm/NewThreadForm.tsx # packages/commonwealth/server/api/external-router.ts
There was a problem hiding this comment.
Maybe fine for this PR but I would like a follow-up ticket for removing all the as unknown as Topic references on the client. From what I can see these are used because of the date <> string type mismatch issue.
We previously agreed that future instances of such errors would be resolved by stopping Sequelize from parsing ISO strings from the DB into JS date objects (make Sequelize leave them as strings). You can see examples of this in libs/model/src/models/community_alerts.ts.
Link to Issue
Closes: #9015
Closes: #9103
Description of Changes
Test Plan