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

Hooks - basic plumbing #528

Merged
merged 2 commits into from
Dec 3, 2018
Merged

Hooks - basic plumbing #528

merged 2 commits into from
Dec 3, 2018

Conversation

timleslie
Copy link
Contributor

@timleslie timleslie commented Nov 27, 2018

This PR implements the basic plumbing for the mutation hooks. It puts in place field type hooks and custom user hooks for fields and lists.

This PR should be considered a WIP for the full hook implementation and should not be used in production (unless you're extremely brave!)

The following things still need to be completed in future PRs:

  • API: The current API has some subtle variations from the documented API and may yet change again. I would like to make this the subject of a future hairy session before finalising.
  • Nested after...() hooks (Hooks - nested afterChange/afterDelete #533). These hooks should not be executed until all nested mutations have been completed. This will require a separate change to how we pass things through to nested mutations.
  • Error handling. The data passed back through validation errors is a basic prototype. Updates to the admin-ui which handle these errors will inform future updates to this API. Failures within other hooks are not currently caught, and will propagate like any other uncaught exception. We want to catch these and serve them back with custom information to inform the app developer and user that it was a hook operation which failed.
  • Rollbacks (Hooks - rollback on failure #534): We don't currently support rolling back when a hook fails.
  • Testing: We don't have any explicit tests for hooks yet. These will be implemented once the API has been finalised.
  • Documentation: Once the API is settled, the docs will need to be updated to reflect this and written in a more consumable format than the current raw notes from the hairy session.

@timleslie timleslie force-pushed the list-validation-hooks branch 5 times, most recently from 3de27cb to ed4b85f Compare December 2, 2018 23:42
@timleslie timleslie changed the title [WIP] Hooks Hooks - basic plumbing Dec 3, 2018
@timleslie timleslie requested a review from jesstelford December 3, 2018 00:03
Copy link
Contributor

@jesstelford jesstelford left a comment

Choose a reason for hiding this comment

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

Looking good so far!

@@ -82,21 +85,21 @@ class Field {
* @param context {Mixed} The GraphQL Context object for the current request
*/
// eslint-disable-next-line no-unused-vars
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// eslint-disable-next-line no-unused-vars

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

hooks: {
validateInput: async ({ resolvedData, addValidationError }) => {
if (resolvedData.name === 'Barry') {
addValidationError("Sorry, no Barry's allowed", { a: 1 }, { b: 2 });
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
addValidationError("Sorry, no Barry's allowed", { a: 1 }, { b: 2 });
addValidationError("Sorry, no Homers allowed", { a: 1 }, { b: 2 });

Simpsons reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 😆

async _resolveDefaults(data) {
const fields = this.fields.filter(field => field.getDefaultValue() !== undefined);
return {
...(await this._mapToFields(fields, field => field.getDefaultValue())),
Copy link
Contributor

Choose a reason for hiding this comment

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

This will execute getDefaultValue() twice (once for the filter, and again for the map). Is there a way we can avoid this in case there are potentially expensive lookups performed, or the function has side effects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree it's a bit icky. I've kept it like this for now just to make the field-mapping pattern clear. I'm open to changing it up once everything settles down a bit.

await this._mapToFields(fields, field => field.validateInput(args));
await this._mapToFields(fields.filter(field => field.config.hooks.validateInput), field =>
field.config.hooks.validateInput(args)
);
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 Since the only output of these two functions is to call addFieldValidationError, which pushes the given data onto fieldValidationErrors, could we not have the validateInput methods return errors instead?

fieldValidationErrors.push(
  ...(await Promise.all(fields.map(field => field.validateInput(args))))
);

Similarly for the other functions below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this mechanism was as per the design in the hairy session. I'll leave a FIXME in the code for us to loop back to this and check whether this is what we really want.

@timleslie timleslie force-pushed the list-validation-hooks branch from ed4b85f to c021c4f Compare December 3, 2018 04:12
@timleslie timleslie merged commit 989a2ff into master Dec 3, 2018
@timleslie timleslie deleted the list-validation-hooks branch December 3, 2018 04:19
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.

2 participants