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 methods getOrDefault and getOrThrow #307

Merged
merged 8 commits into from
May 24, 2024
Merged

Add methods getOrDefault and getOrThrow #307

merged 8 commits into from
May 24, 2024

Conversation

craigbeck
Copy link
Contributor

Added methods for common patterns found in production code:

Data we know we should have in app:

const value = model.get('_page.user')!;
// following code expects value is defined
// will error in less obvious ways as unlikely to handle the cases of undefined

using getOrThrow

const value = model.getOrThrow('_page.user');
// following code will not be reached
// does not need to defensively handle undefined case

Data we may or may not have but want to cleaner code

const files = model.get('files') || [];

using getOrDefault

const files = model.getOrDefault('files', []);

@craigbeck craigbeck requested a review from ericyhwang May 22, 2024 21:38
@craigbeck craigbeck added the minor Increment the minor version when merged label May 22, 2024
const value = this.get(subpath);
if (value === undefined) {
const fullpath = [this._at, subpath].filter(Boolean).join('.');
throw new Error(`No value at path ${fullpath}`)
Copy link

@colincollerlever colincollerlever May 23, 2024

Choose a reason for hiding this comment

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

Is there a way we can include the collection name in the error? I don't know if it's really needed, if we get the call stack, but it might be helpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

The full path will already include the collection name - Model#_at is the child model's absolute path from the root.

src/Model/collections.ts Outdated Show resolved Hide resolved
src/Model/collections.ts Outdated Show resolved Hide resolved
src/Model/collections.ts Outdated Show resolved Hide resolved
src/Model/collections.ts Outdated Show resolved Hide resolved
@@ -152,6 +168,19 @@ Model.prototype.getOrCreateDoc = function(collectionName, id, data) {
return collection.getOrCreateDoc(id, data);
};

Model.prototype.getOrDefault = function<S>(subpath: Path, defaultValue: S) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Mentioning for discussion - as this is now, it won't support a 1-arg call like getOrDefault(defaultValue), though there is the workaround of doing getOrDefault('', defaultValue).

We can always add it later if needed and we don't want to add it now.

Choose a reason for hiding this comment

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

looks like the intent is as a fallback in case the subpath fails? . which would make sense then to keep the structure... I'd argue that providing blank subpath or defaultValue raise error no? They are both required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eric and I discussed ofline and agree the no-subpath case is unlikely, so leaving as is and see if any use comes up.

test/Model/collections.js Outdated Show resolved Hide resolved
@craigbeck craigbeck merged commit aec8f81 into master May 24, 2024
7 checks passed
@craigbeck craigbeck deleted the better-get branch May 24, 2024 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants