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

feat: make req partial and optional in DB / Local API operations #9935

Merged
merged 1 commit into from
Dec 19, 2024

Conversation

r1tsuu
Copy link
Member

@r1tsuu r1tsuu commented Dec 12, 2024

What?

Previously, the req argument:
In database operations (e.g payload.db) was required and you needed to pass the whole req with all the properties. This is confusing because in database operations we never use its properties outside of req.transactionID and req.t, both of which should be optional as well.

Now, you don't have to do that cast:

payload.db.findOne({
  collection: 'posts',
  req: {} as PayloadRequest,
  where: {
    id: {
      equals: 1,
    },
  },
})

Becomes:

payload.db.findOne({
  collection: 'posts',
  where: {
    id: {
      equals: 1,
    },
  },
})

If you need to use transactions, you're not required to do the as cast as well now, as the req not only optional but also partial - Partial<PayloadRequest>.
initTransaction, commitTransaction, killTransaction utilities are typed better now as well. They do not require to you pass all the properties of req, but only payload - MarkRequired<Partial<PayloadRequest>, 'payload'>

const req = { payload }
await initTransaction(req)
await payload.db.create({
  collection: "posts",
  data: {},
  req
})
await commitTransaction(req)

The same for the Local API. Internal operations (for example packages/payload/src/collections/operations/find.ts) still accept the whole req, but local ones (packages/payload/src/collections/operations/local/find.ts) which are used through payload. now accept Partial<PayloadRequest>, as they pass it through to internal operations with createLocalReq.

So now, this is also valid, while previously you had to do as cast for req.

const req = { payload }
await initTransaction(req)
await payload.create({
  collection: "posts",
  data: {},
  req
})
await commitTransaction(req)

Marked as deprecated PayloadRequest['transactionIDPromise'] to remove in the next major version. It was never used anywhere.
Refactored withSession that returns an object to getSession that returns just ClientSession. Better type safety for arguments
Deduplicated in all drizzle operations to getTransaction(this, req) utility:

const db = this.sessions[await req?.transactionID]?.db || this.drizzle

Added fallback for throwing unique validation errors in database operations when req.t is not available.

In migration up and down functions our req is not partial, while we used to passed req with only 2 properties - payload and transactionID. This is misleading and you can't access for example req.t.
Now, to achieve "real" full req - we generate it with createLocalReq in all migration functions.

This all is backwards compatible. In all public API places where you expect the full req (like hooks) you still have it.

Why?

Better DX, more expected types, less errors because of types casting.

@r1tsuu r1tsuu requested a review from DanRibbens December 12, 2024 18:00
@r1tsuu r1tsuu force-pushed the refactor/optional-req-in-db-methods branch from 9da022f to ae91780 Compare December 12, 2024 19:50
@r1tsuu r1tsuu force-pushed the refactor/optional-req-in-db-methods branch from ae91780 to c61ee84 Compare December 12, 2024 19:51
Copy link
Contributor

@DanRibbens DanRibbens left a comment

Choose a reason for hiding this comment

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

Nice changes!

@DanRibbens DanRibbens merged commit 0e5bda9 into main Dec 19, 2024
56 checks passed
@DanRibbens DanRibbens deleted the refactor/optional-req-in-db-methods branch December 19, 2024 03:43
r1tsuu added a commit that referenced this pull request Dec 19, 2024
Regression from #9935, we need
to call `req.t` with `''error:valueMustBeUnique''` instead of just
`''error:valueMustBeUnique''`
Copy link
Contributor

🚀 This is included in version v3.10.0

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

Successfully merging this pull request may close these issues.

2 participants