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

Teach ts sdk about product utility members #236

Merged
merged 3 commits into from
Oct 8, 2019
Merged

Conversation

DanielCollins
Copy link
Contributor

No description provided.

throw new Error("company is undefined, did you forget to embed it?");
}
if (this.company.defaultCurrency === undefined) {
const err = "company.defaultCurrency is undefined, did you forget to" +
Copy link
Collaborator

Choose a reason for hiding this comment

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

default currency is a primary variable how can it not be embeded

}

public taxType = () => {
if (this.domain === undefined) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can wrap in a function checkDomainEmbeded()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not until typescript supports "assertion signatures" (i.e., 3.7)

}

public hasGroupVariationFields = () => {
if (this.groupVariationFields === undefined) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

ideally can be warp in a function checkEmbeded(this.groupVariationFields) or do some magic on property annotations.

Not required for this pr, but just raise this issue #241

const variationFields = variationField.independent ?
this.independentVariationFields : this.groupVariationFields;
const index = variationFields.findIndex(v => {
if (v.id === undefined) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

how could a miss embed cause variation.id to be null.

}

public buildEmptyVariation = () => {
if (this.defaultValue === undefined) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

how could a embed missing lead to undefined "defaultValue"

if (this.defaultValue === undefined) {
throw new Error("defaultValue is undefined, did you forget to embed it?");
}
if (this.variationCost === undefined) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

how could a embed missing lead to undefined "variationCost"

for (const option of this.options) {
if (option.default) {
if (option.variationCost === undefined) {
throw new Error("option.variationCost is undefined, did you " +
Copy link
Collaborator

Choose a reason for hiding this comment

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

same suspection why do embed check on primary value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, the error suggestion is bad, it's unlikely to be from a missed embed.

the situation will more likely arise due creating a new object that hasn't fetched anything at all from the server yet.

Copy link
Collaborator

@s3341458 s3341458 left a comment

Choose a reason for hiding this comment

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

Have questions as I can see embed check on primary attributes.

@s3341458
Copy link
Collaborator

s3341458 commented Oct 4, 2019

as currently our sdk is only used internally, I think the error does not matter that much as long as we know what we are doing

@s3341458 s3341458 merged commit 1b74c6e into master Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants