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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 1, 2017
+1 −1
Merged

#7959 - Fix for JS error on Swatch Renderer for undefined oldPrice #9776

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/code/Magento/Swatches/view/frontend/web/js/swatch-renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,7 +711,7 @@ define([
}
);

if (result.oldPrice.amount !== result.finalPrice.amount) {
if (typeof result != 'undefined' && result.oldPrice.amount !== result.finalPrice.amount) {
Copy link

Choose a reason for hiding this comment

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

Two things about this:'

  1. Always use strict !== comparison rather than loose != comparison, since the coercion rules of the latter can be surprising.
  2. Under what circumstances would result not exist? Can we eliminate that circumstance?

Copy link
Contributor

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."

Copy link
Contributor

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

Copy link
Contributor

@aholovan aholovan Jun 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @OZZlE , you are right about unnecessary strict comparison.

@zetlen, 'result' is undefined before all of the swatches are selected. It was the issue as with oldPrice and also with tierPrices. (how to reproduce is described in #8923 pull request)

Copy link

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!

$(this.options.slyOldPriceSelector).show();
} else {
$(this.options.slyOldPriceSelector).hide();
Expand Down