review changes

[rev-doshi]

Proposed changes

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update

Checklist

• I have read the CONTRIBUTING doc
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)
• I have added proper type definitions for new commands (if appropriate)

Further comments

Reviewers: @webdriverio/project-committers

[sriteja777]

LGTM

[sriteja777]

Won't the first two conditions always evaluate to false?

[rev-doshi]

No, its correct, the variable is a string and it is null check, have tested it

[sriteja777]

I mean the first two conditions are _percyAutoCaptureMode && !_percyAutoCaptureMode. It's like a & !a which always evaluates to false. So is that condition even necessary?

[rev-doshi]

It does always evaluate to undefined or false
But it is acting a null check for the next expression ->
CAPTURE_MODES.includes(_percyAutoCaptureMode as string)

[sriteja777]

Nope. It's not. It's evaluating to (a && b) || c. How is the above condition guaranteeing null check on a?

[rev-doshi]

Fixed

[sriteja777]

Why it is not at top?

[rev-doshi]

https://www.typescriptlang.org/docs/handbook/namespaces-and-modules.html

namespaces/modules are used to organise code and declarations, import statement outside doesn't make sense and hence the build will fail too if we are to import outside

[sriteja777]

I couldn't find any reference in the above link that says imports should be inside namespace declaration. What exact build error does it give if we do the imports outside?

[rev-doshi]

Fixed