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

Handle “undefined” as passed option values #146

Merged
merged 1 commit into from
Feb 10, 2025

Conversation

denis-sokolov
Copy link
Contributor

Allow the consumers to pass “undefined” as options values instead of misbehaving.

Today if the consumer passes “undefined”, we ignore the default options and allow the “undefined” value to fall through to the rest of the program, resulting in undefined behavior. For an example, the maxDelta becomes NaN if threshold: undefined is provided.

After this change we ensure that “undefined” as options values is converted into a default value.

It is customary in JavaScript, especially in options objects, to treat a missing key and a key with value of undefined the same way. In particular, it is a distinction that is easily introduced when the customer receives a value from somewhere else and wants to continue to support default values.

Related to #14, #52.

Alternative syntax to consider:

const {
    threshold = 0.1,         // matching threshold (0 to 1); smaller is more sensitive
    includeAA = false,       // whether to skip anti-aliasing detection
    alpha = 0.1,             // opacity of original image in diff output
    aaColor = [255, 255, 0], // color of anti-aliased pixels in diff output
    diffColor = [255, 0, 0], // color of different pixels in diff output
    diffColorAlt = null,     // whether to detect dark on light differences between img1 and img2 and set an alternative color to differentiate between the two
    diffMask = false         // draw the diff over a transparent background (a mask)
} = options;

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Thanks! I think I'd prefer the simpler destructuring syntax — values will drop the options namespace this way but this should be OK for our purposes.

@denis-sokolov
Copy link
Contributor Author

Updated.

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Looks good to me! Perhaps worth adding a minimal test to validate the fix too?

Allow the consumers to pass “undefined” as options values instead of misbehaving.

Today if the consumer passes “undefined”, we ignore the default options and allow the “undefined” value to fall through to the rest of the program, resulting in undefined behavior. For an example, the maxDelta becomes NaN if threshold: undefined is provided.

After this change we ensure that “undefined” as options values is converted into a default value.

It is customary in JavaScript, especially in options objects, to treat a missing key and a key with value of undefined the same way. In particular, it is a distinction that is easily introduced when the customer receives a value from somewhere else and wants to continue to support default values.
@denis-sokolov
Copy link
Contributor Author

Added a test.

@mourner mourner merged commit bf7001f into mapbox:main Feb 10, 2025
1 check passed
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