Make passing in the params by model api identifier optional #178
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Nice. This deals with the internal API. Do we have plans to do something similar for the public API? |
|
We certainly do! https://github.com/gadget-inc/gadget/pull/6455 |
thegedge
reviewed
Apr 21, 2023
jasong689
force-pushed
the
optional-api-identifier
branch
3 times, most recently
from
f083e9d
to
95da719
Compare
jasong689
force-pushed
the
optional-api-identifier
branch
2 times, most recently
from
f018847
to
bd350a3
Compare
|
ready for another review. this was updated to throw an error if the flat style is used and the model has ambiguous identifiers |
thegedge
reviewed
May 12, 2023
| } | ||
|
|
||
| if (this.apiIdentifier in record) { | ||
| recordData = recordData[this.apiIdentifier]; |
There was a problem hiding this comment.
👍 looks like we don't have any codepath that could have extra params in the internal API right now, but we'll have to remember that if that changes in the future this would potentially break (since it would clobber those params). Very likely we'd catch that kind of change in CI.
| ) { | ||
| this.capitalizedApiIdentifier = camelize(apiIdentifier); | ||
| } | ||
|
|
||
| private validateRecord(record: RecordData) { | ||
| if (!this.options || !this.options.hasAmbiguousIdentifiers) { |
jasong689
force-pushed
the
optional-api-identifier
branch
from
bd350a3
to
e440850
Compare
thegedge
approved these changes
May 17, 2023
…ate mutations optional
jasong689
force-pushed
the
optional-api-identifier
branch
from
e440850
to
800ddcf
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR makes it optional to wrap your model params for create/update in the model api identifier. The only exception is if there is a field with the same api identifier as the model, then the params must be wrapped in the model api identifier; this is because there's no way to tell if the consumer is passing in model params or field value.
It would be nice to be able to test that the correct query/variables are called but the mutations rely on transactions via ws. This is a bit difficult to mock I think without adding some interfaces so that we can create a mock transaction, not sure if that was worth adding here.
PR Checklist