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

Add: async creation of items #2146

Closed
wants to merge 8 commits into from
Closed

Add: async creation of items #2146

wants to merge 8 commits into from

Conversation

andrewlinfoot
Copy link
Contributor

This is a WIP to tackle issues: keystonejs/keystone#1705, keystonejs/keystone#1713, and keystonejs/keystone#1714. I'd love to get some feedback on it.

I just implemented async validation and left the .updateItem method alone. If this approach looks like the right direction, I'll make the updateItem methods async as well. From there, it should be easy to add the .processUpdateData api and handle file uploads.

A couple questions about the API in general:

  1. Should the .validateInput api return more information than just false when there is an invalid input? I'm having trouble seeing how we would implement more granular error messages with the API as is.
  2. Should the files be passed to the .validateInput method too or should file validation happen somewhere else?
  3. Is it necessary to pass required to .validateInput(data, required, item, callback) when the field has the property .required and it can be accessed with this.required in the .validateInput method?

Overall, this new API seems way cleaner than the UpdateHandler approach.

@andrewlinfoot
Copy link
Contributor Author

I submitted a different PR that made all of the updateItem methods async. That is merged in and good to go (keystonejs/keystone#2147).

I have some free time tonight and tomorrow and am planning on tackling the .processUpdateData API and polishing off the new update and create API endpoints. I would love to get some thoughts on this PR before I jump too deep into implementing the rest of the validation errors and adding support for file uploads. @JedWatson @morenoh149 @creynders @webteckie @ericelliott any thoughts?

@andrewlinfoot andrewlinfoot changed the title Add: validation errors for create API Add: async creation of items Jan 28, 2016
@andrewlinfoot
Copy link
Contributor Author

Just pushed some updates that implement a processFormInput method and change the validateInput method to call its callback with an error object instead of just true or false. This seems like a pretty solid API and should allow for file uploads, async data updating, async data validation and fine grained error messages defined by the FieldTypes themselves. Assuming I can get some consensus around this API, I'm going to start implementing processFormInput methods for some of the types that depend on file uploads. @JedWatson thoughts?

@andrewlinfoot
Copy link
Contributor Author

Commit keystonejs/keystone@a73d131 adds a minimal version of processFormData for CloudinaryImage types

@andrewlinfoot
Copy link
Contributor Author

Newest commits by Jed implement this API in a cleaner way. Closing the PR.

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

Successfully merging this pull request may close these issues.

1 participant