-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Singleton list #2838
Singleton list #2838
Conversation
🦋 Changeset is good to goLatest commit: 1107ac9 We got this. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -296,7 +296,7 @@ module.exports = class Keystone { | |||
throw new Error(`Invalid list name "${key}". List names cannot start with an underscore.`); | |||
} | |||
|
|||
const list = new List(key, compose(config.plugins || [])(config), { | |||
const list = new List(key, compose(config.plugins || [])({ ...config, key, keystone: this }), { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding key
and keystone
allows use of this inside the plugin, not sure if this is a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting change, I can see why it's needed in this case. There are ways you could work around needing key
and keystone
, but I think it makes more sense to pass them through as a convenience.
Because we're doing a compose
operation it means that every plugin function will need to make sure it returns these values correctly as well, which I think perhaps is not ideal.
Maybe the best change is to change the signature of the plugin functions from:
{...config} => {...config}
to
{ key, keystone, config } => config
That way the plugin writer can still just return the config object (the thing the plugin is responsible for) and we can update the code on this line to handle the composition and making sure the right things get pass through to the right places.
This change would be a major
breaking change to both @keystonejs/keystone
and keystonejs/list-plugins
(which is fine).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured we can best do with changing the compose function,
we will pass on two args to each plugin, it is upto them what to use.
(config, {listKey, keystone}) => config;
caveat is that if we give them {...config, listKey, keystone } => ?
then they may not return listkey and keystone for next plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#3011 achieves this. I have used listKey as this is being used many places. name
/listName
may also be more suitable instead of listKey.
}) => { | ||
const newResolveInput = async ({ resolvedData, operation }) => { | ||
if (operation === 'create') { | ||
const list = keystone.getListByKey(key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this and executeQuery
scenario is enabled by passing key
and keystone
to the plugin config. These are discarded by the List constructor anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just for this plugin, the keystone parameter can be reverted and We could construct the query using all${key}Meta
instead of using listQueryMetaName
and use action: { query }
of hook argument rather than keystone.executeQuery
const { | ||
data: { [list.gqlNames.listQueryMetaName]: listQuery } = {}, | ||
errors, | ||
} = await keystone.executeQuery(query); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually going to use action: { query }
parameter but realised that keystone is already available. Should I change this to query
parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @gautamsi, this looks like it's heading in a really good direction. I'd like to work on getting this over the line.
There are a couple of things I think we should factor out as separate PRs first (the plugins
function API with key
and keystone
and disabling the delete
button in the footer based on access control). There are a few other questions and commends I've left through the code which will hopefully get us a closer to a nice tight solution 👍
@@ -296,7 +296,7 @@ module.exports = class Keystone { | |||
throw new Error(`Invalid list name "${key}". List names cannot start with an underscore.`); | |||
} | |||
|
|||
const list = new List(key, compose(config.plugins || [])(config), { | |||
const list = new List(key, compose(config.plugins || [])({ ...config, key, keystone: this }), { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting change, I can see why it's needed in this case. There are ways you could work around needing key
and keystone
, but I think it makes more sense to pass them through as a convenience.
Because we're doing a compose
operation it means that every plugin function will need to make sure it returns these values correctly as well, which I think perhaps is not ideal.
Maybe the best change is to change the signature of the plugin functions from:
{...config} => {...config}
to
{ key, keystone, config } => config
That way the plugin writer can still just return the config object (the thing the plugin is responsible for) and we can update the code on this line to handle the composition and making sure the right things get pass through to the right places.
This change would be a major
breaking change to both @keystonejs/keystone
and keystonejs/list-plugins
(which is fine).
|
||
| Option | Type | Default | Description | | ||
| --------------- | --------- | ------- | ------------------------------------------------- | | ||
| `preventDelete` | `Boolean` | `true` | Prevents deletion of the (only) item in the list. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we ever want to allow someone to delete the single item in the list? In my mind if you have a singleton item, say Settings
, with a bunch of fields, does it make sense to ever delete it?
And on a similar line, do we want to support create
on this list, or would it make more sense to make update
the only operation, and tweak it to perform a create
operation if the item doesn't already exist in the database?
That way the access control on the list could be pinned down to { create: false, delete: false }
, which might conceptually simplify what this plugin allows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are right, we can keep this restricted to create only when there is no item.
one caveat is that if create
is set to false it is rejected before we hit any hooks.
one improvement would be that we pre-create this item, which I don't see a way without having an event system like keystone.on('connect', () => createItem
from within plugins.
(this event was there in v4, if I remember it right)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we do create-object-if-missing operation during the update
mutation, rather than having it be a system startup thing (which is a pattern I'd rather avoid in general).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does that work? update mutation needs an ID and in admin-ui you can not update an item which is not there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create on read is another possibility? there is no read hook for list. will have to add infrastructure for this to create an item if list is empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I see that you're right about update needing an ID
. I think you're right, we probably do need to support the create
operation, anything other than that will require too much jumping through hoops.
Could you update the PR so that it's got { delete: false }
enforced and then we can see what if there's anything else that needs tweaking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
working...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -95,7 +102,7 @@ export default memo(function Footer(props) { | |||
<div> | |||
<Button | |||
appearance="danger" | |||
isDisabled={updateInProgress} | |||
isDisabled={updateInProgress || !canDelete} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it would be a good addition to the admin-ui, independent of the rest of the singleton changes. Could you break it out into a single PR?
I'd probably spell it:
const { list } = useList();
...
isDisabled={updateInProgress || !list.access.create}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
openCreateItemModal, | ||
} = useList(); | ||
if (!access.create) return null; | ||
if (!access.create || singleton) return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we force { create: false }
for the access on this list (see other comment), then we don't need to make this change as we'll get the behaviour we want for free.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right.
ref #1639
you can now use this plugin to prevent adding more than one item in a list and also disallow deletion of the item in the list.
admin ui changes:
I am also inclined to make another plugin which would limit
minItems
andmaxItems
to the list, it could be a part of list config natively, but adding a plugin should also be good.TODO:redirect from list page to item pageremoveCreate
button when there is an item in the list.remove delete button (should really be addressed separately as there is no way to prevent delete button based on access for now)- Disabled the button rather than removing it.