fix(core): It is possible for featuredAsset to resolve to {id: ID} #3177
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Hmm. These failing tests certainly suggest this isn't the correct solution for backend, but I'm at a bit of a loss since it works fine in my code. I'm attempting to reproduce the failure on my system so I can hopefully address it. |
|
I am unable to reproduce this issue running e2e tests on my system and would appreciate any guidance. |
| @@ -32,6 +34,18 @@ export class OrderLineEntityResolver { | |||
| @Parent() orderLine: OrderLine, | |||
| ): Promise<Asset | undefined> { | |||
| if (orderLine.featuredAsset !== undefined) { | |||
There was a problem hiding this comment.
I believe the tests are failing because you are doing an exact !== check for undefined. So if orderLine.featuredAsset is null, then the "if" statement will be true, and the program will go to line 40 and attempt to access featuredAsset.preview, which results in the error you see in the e2e tests:
Error: Cannot read properties of null (reading 'preview')
So the solution should be to use !=, which will check for both null and undefined.
taxilian
force-pushed
the
fix/featuredAssetMissing
branch
2 times, most recently
from
43419f3
to
e0665ef
Compare
|
we'll give that a try... if we're just checking for "truthyness" I'd rather it was specifically doing that, rather than the != which does the same thing but doesn't look like it. |
| @@ -31,7 +33,19 @@ export class OrderLineEntityResolver { | |||
| @Ctx() ctx: RequestContext, | |||
| @Parent() orderLine: OrderLine, | |||
| ): Promise<Asset | undefined> { | |||
| if (orderLine.featuredAsset !== undefined) { | |||
| if (!!orderLine.featuredAsset) { | |||
There was a problem hiding this comment.
I thought more about this and I think it can be simplified even further to:
if (!orderLine.featuredAsset?.preview) {
return orderLine.featuredAsset;
} else {
return this.assetService.getFeaturedAsset(ctx, orderLine);
}reasoning:
- less code
- fewer logical paths == easier to reason about
- if any future refactors occur to how we fetch a featured asset, they will automatically apply to this code path == easier maintenance.
The comment is still worth having though to explain why we explicitly check for the presence of the preview property.
There was a problem hiding this comment.
Yeah, that makes sense; I was stuck on "change the least you need to" thinking that if featuredAsset is not truthy then we should avoid calling getFeaturedAsset, but I trust that you know better than I if there are any legitimate cases where that would happen, so I'll make and push the change.
There was a problem hiding this comment.
unless I'm reading wrong, though, you have your logic backwards, and it should be:
if (!orderLine.featuredAsset?.preview) {
return this.assetService.getFeaturedAsset(ctx, orderLine);
} else {
return orderLine.featuredAsset;
}
e0665ef
to
2452bf0
Compare
|
Description
We experienced an issue with trying to add products to an existing order; when we added a new product which was not previously in the order, the featuredAsset would resolve to an object with an ID and nothing else. We have this fix locally as a resolver, but it's simple and innocuous enough I thought I'd submit it for your consideration to help others.
If this isn't the right way to do this I apologize; we're still new-ish to this project. Let me know how to do it better and I'll fix it =]
Breaking changes
None
Screenshots
N/A
Checklist
📌 Always:
👍 Most of the time:
- (this might not be a bad idea, but I don't know how to reproduce it with a test case)