Make require: true the default for Model#fetch#2006
Merged
Conversation
- It's not easy catching collection specific fetch errors at the moment, so that change would probably cause more problems than the ones it would solve.
Merged
10 tasks
|
This is not a good change. You're making quite an assumption for use cases. What if I wanted to check if a value exists (e.g., I'm searching by string for a dictionary entry... if it's not there, oh well!). You should leave it to the developer to decide what they want to do if the value is not there. I upgraded recently from 0.13 to 1.0 and this broke many things. |
This file contains hidden or 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
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.
Introduction
Make
require: truethe default forModel#fetchandCollection#fetchOnecalls.Motivation
When fetching a single model there is usually a precise set of constraints, so as a user I'm interested in getting that model, not any other value. This is especially true when logging in a user, or when looking for a specific record of something. Usually all further processing of said model should be skipped since it wouldn't make sense, so this facilitates that mechanism by default.
This change ensures that fetch calls like:
will always resolve with a model, avoiding the need to additionally check if the result is not
null. If the model cannot be fetched because it does not exist in the database an error will be thrown and thefetchcall will be rejected withMyModel.NotFoundError. This specific error class can be caught and dealt with appropriately:Closes #718.
Proposed solution
This adds a new
Model#requireFetchproperty that can be used to set the default behavior when handling empty responses ofModel#fetch, and sets its default value totrue, meaning that the default is to reject thefetchPromise.The new model level property also means that users that prefer the previous behavior can easily adapt their code by simply adding
requireFetch: falseto their model definitions.There is a migration guide in the Wiki to help with the transition to this new default.
Current PR Issues
This does not change the default behavior when fetching collections, mainly because it's not easy to catch an
EmptyResponseerror from a collection built from model methods likeModel#fetchAllorModel#fetchPage. It can also be argued that when fetching collections, the requirements are not as strict, and it's usually OK for them to be empty.Not everyone is going to like this change, but it should be relatively easy to keep using the previous behavior for those that prefer to think is terms of SQL and less about abstract higher level objects.
Alternatives considered
There are some commits in this series that actually made
require: truethe default also for fetching collections, unifying all fetch interfaces in terms of behavior, but it was proving very difficult (impossible?) to catch errors by a specific collection'sEmptyResponseerror.