-
Notifications
You must be signed in to change notification settings - Fork 82
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 implementation to getFieldValue #83
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a subset of the impl from lwc-recipes. Why not use the real impl?
@kevinv11n I updated the stub here to use the real impl. Also added the impl for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few minor suggestions
* @return The object and field API names. | ||
*/ | ||
function splitQualifiedFieldApiName(fieldApiName) { | ||
const idx = fieldApiName.indexOf("."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider adding existence check prior invoking indexOf
/** | ||
* Split the object API name and field API name from a qualified field name. | ||
* Eg: Opportunity.Title returns ['Opportunity', 'Title'] | ||
* Eg: Opportunity.Account.Name returns ['Opportunity', 'Account.Name'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can these be namespaced?
// object api name must non-empty | ||
throw new TypeError("Value does not include an object API name."); | ||
} | ||
return [fieldApiName.substring(0, idx), fieldApiName.substring(idx + 1)]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about returning an object with apiName and fieldName instead of an array? This way you don't have to worry about the order and the readability will be more straight forward
const idx = fieldApiName.indexOf("."); | ||
if (idx < 1) { | ||
// object api name must non-empty | ||
throw new TypeError("Value does not include an object API name."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should include the fieldApiName in the error message
if (fvr === undefined) { | ||
return undefined; | ||
} | ||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is else purposely on a new line?
if () {
...
} else {
...
}
* @return The qualified field API name. | ||
*/ | ||
function getFieldApiName(value) { | ||
if (typeof value === "string") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we care about an empty string here?
else if (value && typeof value.objectApiName === "string" && typeof value.fieldApiName === "string") { | ||
return value.objectApiName + "." + value.fieldApiName; | ||
} | ||
throw new TypeError("Value is not a string or FieldId."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include the value in the error message
@apapko I should have made this more clear in the PR description but this is code from the actual implementation of these stubbed out methods. I don't want to tweak it in minor ways. We should keep it a straight copy/paste so if the real impl gets updated we can update the code here trivially. |
Fixes #79