Implement optional attribute names. Fixes #86 #91
Conversation
1 similar comment
There was a problem hiding this comment.
Overall looks good, and my comments are about fairly minor things.
But there are two things that caught my attention:
- Mutating the input object in
transformSerializedNamesToPropertyNamesmakes me a bit uneasy, and can lead to data loss as I described in the comment. - If I understood it correctly,
getAttributePropertyNameswill be called for every object being deserialized, which will then loop through all properties to rebuild that name/property map. Feels a bit wasteful.
Thinking about it, maybe the deserialization/renaming logic could be moved to JsonApiModel's constructor, replacing its Object.assign() with a loop through the declared attributes. I believe this could provide a number of benefits:
- The
dataparameter toJsonApiModel's constructor could be defined as being the raw object received from the server. Nothing more, nothing less. (i.e. attributes names and values are in their raw form) - No intermediary objects or loops would be needed.
- No risk of data loss during mutation, even if the attribute names and serialized names form some kind of tangle.
Sorry if this explanation is not clear, I could make a simple commit on top of this one to demonstrate.
src/models/json-api.model.ts
Outdated
| import find from 'lodash-es/find'; | ||
| import { Observable } from 'rxjs/Observable'; | ||
| import { JsonApiDatastore, ModelType } from '../services/json-api-datastore.service'; | ||
| import {isPropertyAssignment} from 'codelyzer/util/astQuery'; |
There was a problem hiding this comment.
I’m guessing this was an accidental import?
src/models/json-api.model.ts
Outdated
| if (data) { | ||
| this.id = data.id; | ||
| Object.assign(this, data.attributes); | ||
| if (data.attributes) { |
There was a problem hiding this comment.
Any particular reason for this test? I just checked the spec, and it says it’s safe to pass null or undefined.
| import * as dateParse from 'date-fns/parse'; | ||
|
|
||
| export function Attribute(config: any = {}) { | ||
| export function Attribute(serializedName: string = null) { |
There was a problem hiding this comment.
This would fail under strictNullChecks. Could it instead be an optional?
export function Attribute(serializedName?: string) {| for (let propertyName in attributesMetadata) { | ||
| if (attributesMetadata.hasOwnProperty(propertyName)) { | ||
| let metadata: any = attributesMetadata[propertyName]; | ||
| console.log(metadata); |
| Object.keys(serializedNameToPropertyName).forEach(serializedName => { | ||
| if (attributes[serializedName]) { | ||
| attributes[serializedNameToPropertyName[serializedName]] = attributes[serializedName]; | ||
| delete attributes[serializedName]; |
There was a problem hiding this comment.
I believe this would completely erase the value if the library user passes the same serialized name as the property name, e.g.:
@Attribute("name") name: string;This could be guarded by either @Attribute or getAttributePropertyNames, but it gets trickier if for whatever reason the developer wants to flip fields around, e.g.:
@Attribute("first-name") name: string;
@Attribute("last-name") surname: string;
@Attribute("name") fullName: string;In this case, by the time the deserializer got to fullName, it would delete the name that was assigned before.
test/models/author.model.ts
Outdated
| name: string; | ||
|
|
||
| @Attribute() | ||
| @Attribute("dob") |
There was a problem hiding this comment.
Should use single quotes, per tslint. (For some reason, though, the lint script is not warning about this)
HennerM
force-pushed
the
optional-parameter-names
branch
from
4970a1f to
7e43266
Compare
Feature to explicitly override attribute names used for serialization.
Can be used to solve use cases like #86 .
@mernen I would appreciate if you also can take a look please.