Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 webhook _id and api_version to webhook handler #1268
Add webhook _id and api_version to webhook handler #1268
Changes from 1 commit
9a9ded8
4ce783d
5a9436a
99d3ceb
aaa46e1
9901507
f2a3eee
39163f0
0aa3f90
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Should we:
data
?data
an object instead of a hash to keep the API similar to what it was before? Can we useShopifyAPI::Webhooks::Request
?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 this name is clear enough - is there something we can name it that indicates what it does (i.e. that it takes in the metadata object, or the full metadata)?
We could even take it a bit further: if we need a new method, we could simply test for the presence of that method in the class when we're calling, so
use_handle_webhook?
wouldn't be necessary.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.
We could even take it a bit further: if we need a new method, we could simply test for the presence of that method in the class when we're calling, so use_handle_webhook? wouldn't be necessary.
This is what I originally tried and a couldn't get this to work, do you know of a way?
This is what I did:
Originally this was an interface file meaning meaning if we add a new method users would need to implement the new method or they get an error (breaking change). So I changed it to an abstract file with a default implementation so users would not have to implement the new method. But then you loose the ability to check with
respond_to?
because there is and implementation so it always returns true.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 right - I guess we can't have an interface with an optional method (unless we create a wholly different interface), which complicates things indeed. Let's go ahead with this approach then.
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.
As discussed, I refactored the handler to have multiple handler modules.
Developers can implement the new handler if they want they additional data.