-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
#7959 - Fix for JS error on Swatch Renderer for undefined oldPrice #9776
#7959 - Fix for JS error on Swatch Renderer for undefined oldPrice #9776
Conversation
@@ -711,7 +711,7 @@ define([ | |||
} | |||
); | |||
|
|||
if (result.oldPrice.amount !== result.finalPrice.amount) { | |||
if (typeof result != 'undefined' && result.oldPrice.amount !== result.finalPrice.amount) { |
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.
Two things about this:'
- Always use strict
!==
comparison rather than loose!=
comparison, since the coercion rules of the latter can be surprising. - Under what circumstances would
result
not exist? Can we eliminate that circumstance?
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.
I wonder why these questions weren't raised when the original fix 269c215 was implemented yet they slow down backporting.
Maybe @aholovan could provide the insights you're looking for?
P.S. To quote some guy reporting an issue here on GitHub, "It's unbelievable the effort you guys go to, to avoid fixing the issue."
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.
typeof always returns a string so it's not necessary or standard practice to use strict comparison
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.
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.
@aholovan Thanks for the background. Making the result
always present is a good idea, but it seems to be out of scope for this PR.
@korostii I look at a fairly random sample of pull requests; my schedule doesn't allow me to review every single one. I didn't notice that this was a backport, so that's on me!
As for strict comparison, it really is a standard practice in all cases. Linters in standard configuration will complain. But it's all right; in a later project, we'll use an automated tool to change all of these loose comparisons to strict ones. So I will approve this!
@dreamworkers thank you for your contribution. Your Pull Request has been successfully merged |
This PR backports commit 269c215 (originally for -develop) for 2.1-develop and issue #7959.
Description
Check and verify if 'result' is defined or not before oldPrice is fetched from it.
Fixed Issues (if relevant)
Manual testing scenarios