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

chore(beta): pass req to defaultValue function and better type for defaultValue #5912

Closed

Conversation

r1tsuu
Copy link
Member

@r1tsuu r1tsuu commented Apr 18, 2024

Description

Currently defaultValue function argument is called only server side, so why pass only locale and user? It shouldn't be a breaking change because req contains them. You also can't use the Local API in defaultValue because of that,.
Improved type for defaultValue, should respect all non-function values and function that accepts PayloadRequest.

  • I have read and understand the CONTRIBUTING.md document in this repository.

Type of change

  • Chore (non-breaking change which does not add functionality)
  • This change requires a documentation update

Checklist:

  • Existing test suite passes locally with my changes
  • I have made corresponding changes to the documentation

@r1tsuu r1tsuu marked this pull request as draft April 18, 2024 18:28
@r1tsuu r1tsuu marked this pull request as ready for review April 18, 2024 18:32
@DanRibbens
Copy link
Contributor

I love the idea of passing req to defaultValue. My only concern is that adding anything in the future, outside of req will get awkward. You would then need additional args or a breaking change to nest req into an object.

I'm considering it may make sense to keep { user, locale } and add req inside.

Maybe passing req is all we will ever need though, I can't think of a scenario that this is the case currently and adding to req is always possible in the future.

@r1tsuu
Copy link
Member Author

r1tsuu commented Apr 19, 2024

I love the idea of passing req to defaultValue. My only concern is that adding anything in the future, outside of req will get awkward. You would then need additional args or a breaking change to nest req into an object.

I'm considering it may make sense to keep { user, locale } and add req inside.

Maybe passing req is all we will ever need though, I can't think of a scenario that this is the case currently and adding to req is always possible in the future.

I thought about that but then you would have data duplication in the argument object, it's not like a big deal but even in for example Access where we are using user object pretty much every time it's still in the req
But i'm open to it

@todor0v
Copy link

todor0v commented Apr 26, 2024

@DanRibbens I have a group meta field used in multiple places. I want to have the defaultValue of a checkbox inside it be true on certain collections and false on others. Is it possible to access collection in addition to req so I can do something like this:

defaultValue: ({req, collection}) => {
	return collection.slug === "target-slug";
}

Unless you have a better suggestion.

@r1tsuu
Copy link
Member Author

r1tsuu commented Apr 26, 2024

@DanRibbens I have a group meta field used in multiple places. I want to have the defaultValue of a checkbox inside it be true on certain collections and false on others. Is it possible to access collection in addition to req so I can do something like this:

defaultValue: ({req, collection}) => {
	return collection.slug === "target-slug";
}

Unless you have a better suggestion.

i think that shouldn't be specfic to defaultValue but rather your own implementation on how are you reusing the group field?
you can always do this

import type { Field } from 'payload/types';
import type { Config } from 'payload-types';

const numberField = (collection: keyof Config['collections']): Field => {
  return {
    defaultValue: (req) => {
      switch (collection) {
        case 'admins':
          return 3;
        case 'posts':
          return 4;
      }

      return 5;
    },
    name: 'someNumber',
    type: 'number',
  };
};

Then in your collection config:

export const Collection: CollectionConfig = {
  fields: [numberField('admins')],
  slug: 'admins',
};

am i wrong about this?

you don't even need to have defaultValue as function here actually.
you can do something like this

// in func
let defaultValue: number = 5;

switch (collection) {
 case "admins":
   defaultValue = 4;
}

return {
  type: "number",
  defaultValue,
  name: "someNumber"
|

@todor0v
Copy link

todor0v commented Apr 26, 2024

@r1tsuu Thank you for the suggestion! This would work, but I have to make a lot of changes to take advantage of it. I have a reusable meta field and I don't want to manually pass the collection I use it in. It is basically this:

export const metaField: Field = {
        name: "meta",
	type: "group",
        ....
        numberField('admins')
}

If I provide a parameter to numberField, I have to convert metaField to be a function and not just a field and receive this parameter anywhere it has been used just to cover this one case. If it is somehow possible to use defaultValue it seems much cleaner to me since that is what it should be responsible for anyway.

@DanRibbens
Copy link
Contributor

I agree with @r1tsuu. This is on the side of your config code and payload doesn't pass the collection config to itself like that. It is better that we don't introduce circular references.

Because you already have a reused field it should be quick to find and replace it or change symbol in your IDE.

@tobiasiv
Copy link

tobiasiv commented May 3, 2024

Is it also possible to pass the search params? I need the id of a parent document as the default value for a drawer document (e.g. set a customer id as default value inside an order).

@r1tsuu
Copy link
Member Author

r1tsuu commented May 3, 2024

Is it also possible to pass the search params? I need the id of a parent document as the default value for a drawer document (e.g. set a customer id as default value inside an order).

This PR solves that as req object contains searchParams.
@DanRibbens what should i do? i can make it to keep user, locale and just pass req as you proposed.

@mykz
Copy link
Contributor

mykz commented Sep 17, 2024

@r1tsuu @DanRibbens Do you guys have a status on this? I currently have a case where I need 'req' to be part of defaultValue.

Copy link
Contributor

This PR is stale due to lack of activity.

To keep the PR open, please indicate that it is still relevant in a comment below.

@r1tsuu
Copy link
Member Author

r1tsuu commented Dec 12, 2024

reworked here #9937

@r1tsuu r1tsuu closed this Dec 12, 2024
@r1tsuu r1tsuu deleted the chore/pass-req-to-default-value branch December 12, 2024 18:46
DanRibbens pushed a commit that referenced this pull request Dec 16, 2024
Rework of #5912

### What?
Now, when `defaultValue` is defined as function you can receive the
`req` argument:
```ts
{
  name: 'defaultValueFromReq',
  type: 'text',
  defaultValue: async ({ req, user, locale }) => {
    return Promise.resolve(req.context.defaultValue)
  },
},
```

`user` and `locale` even though are repeated in `req`, this potentially
leaves some room to add more args in the future without removing them
now.
This also improves type for `defaultValue`:
```ts
type SerializableValue = boolean | number | object | string
export type DefaultValue =
  | ((args: {
      locale?: TypedLocale
      req: PayloadRequest
      user: PayloadRequest['user']
    }) => SerializableValue)
  | SerializableValue
```

### Why?
To access the current URL / search params / Local API and other things
directly in `defaultValue`.

### How?
Passes `req` through everywhere where we call `defaultValue()`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants