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.

Sign up for GitHub

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

Singular Resource Handling (read/write operations) #98

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Singular Resource Handling (read/write operations) #98

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ce2aaad
Update model.ts
bilouw Jun 22, 2021
4306905
handle singular resource read
bilouw Jun 24, 2021
a752352
little fix
bilouw Jun 25, 2021
e55bc33
cleaner code
bilouw Jun 25, 2021
5dd4d2a
fix: condition priority issue
bilouw Jun 25, 2021
5d12968
add: findSingle() function
bilouw Sep 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import {
JsonapiResourceIdentifier
} from "./jsonapi-spec"

import { singularize } from "inflected"
import { cloneDeep } from "./util/clonedeep"
import { nonenumerable } from "./util/decorators"
import { IncludeScopeHash } from "./util/include-directive"
Expand All @@ -65,6 +66,7 @@ export interface ModelConfiguration {
keyCase: KeyCase
strictAttributes: boolean
logger: ILogger
singularResource: boolean
}
export type ModelConfigurationOptions = Partial<ModelConfiguration>

Expand Down Expand Up @@ -167,6 +169,7 @@ export class SpraypaintBase {
static keyCase: KeyCase = { server: "snake", client: "camel" }
static strictAttributes: boolean = false
static logger: ILogger = defaultLogger
static singularResource: boolean = false
static sync: boolean = false
static credentials: "same-origin" | "omit" | "include" | undefined
static clientApplication: string | null = null
Expand Down Expand Up @@ -774,10 +777,14 @@ export class SpraypaintBase {
}

static url(id?: string | number): string {
const endpoint = this.endpoint || `/${this.jsonapiType}`
const endpoint =
this.endpoint ||
(this.singularResource
? `/${singularize(this.jsonapiType!)}`
: `/${this.jsonapiType}`)
let base = `${this.fullBasePath()}${endpoint}`

if (id) {
if (id && !this.singularResource) {
base = `${base}/${id}`
}

Expand Down
2 changes: 1 addition & 1 deletion src/scope.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export class Scope<T extends SpraypaintBase = SpraypaintBase> {
return this._buildCollectionResult(response)
}

async find(id: string | number): Promise<RecordProxy<T>> {
async find(id?: string | number): Promise<RecordProxy<T>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the functionality of this whole PR is good, but as it's an uncommon case I don't love the type change of making id optional to find here, as for 99% of projects a missing id is incorrect and will result in confusing errors that the type system could easily catch. Perhaps we can come up with a parallel method that is only used for singular resources? findOnly() load, i'm not loving those but I'm open to other ideas.

Copy link
Collaborator Author

@bilouw bilouw Jun 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

My idea was to let the singularResource variable determine the way the find() function behave. But i understand your point, i thought about making a parallel method too. does get() suit you for the method name, or is it too generic ?

Also, with my changes calling find('someId') will construct an endpoint url without id if singularResource = true, so maybe another method is not necessary and we could just use the method like this: find('whatever') leaving the id of find() mandatory.

Copy link
Contributor

Choose a reason for hiding this comment

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

what about findSingle?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm agree with this name. I just made the change, can this be merge ?

const json = (await this._fetch(this.model.url(id))) as JsonapiResourceDoc

return this._buildRecordResult(json)
Expand Down