Change return value of feedback.warning when there is no warning given

[U-4-E-A]

Currently returns the following if no warning found: -

feedback: { warning: '', suggestions: [] }

Suggest changing type of warning to string | null or string[]

Reason - empty string still indicates on a programmatic level that a warning "exists". Even though the warning is not currently an array (rather a single string), a return of warning: [] or warning: ["A warning message here"] may be a logical option.

If a string (rather than an array of strings) is to be returned, suggest warning: null or warning: "A warning message here" to establish non-existence of warning. This will allow for if(feedback.warning) {...} rather than if(feedback.warning.length) {...}, which is somewhat hackish.

[MrWook]

I don't really understand this.
An empty string is a falsy value so if(feedback.warning) should work fine 🤔
Additional there will always be only one warning because this is something set by the highest matcher and are not combined same with the suggestions.

[bagbag]

An empty string is a falsy value so if(feedback.warning) should work fine

While it does work, it relies on JavaScripts coercing, which is simply not clean, at least in my view.
Image that you forget to check with an if, then it will just proceed with that empty string. If you have null instead, the TypeScript compiler will complain.

[MrWook]

It was @U-4-E-A improved example with if(feedback.warning), which is ignoring an empty warning just fine.

What is the use case where a typescript compile will throw an error?

I will check with a few colleagues about "clean code" to get some more opinions on this matter.

[bagbag]

I found this library yesterday and added it to my code. I already have other stuff there which is why I merge the results (code simplified):

type CheckPasswordResult = {
  warnings: string[]
};

function examplePasswordCheck(password: string): CheckPasswordResult {
  const zxcvbnResult = zxcvbn(password);
  const someOtherResult = someOtherStuff(password);

  const warnings: string[] = [...someOtherResult.warnings, zxcvbnResult.feedback.warning];
  return { warnings };
}

function consumer(): void {
  const passwordResult = examplePasswordCheck('password-from-request');

  if (passwordResult.warnings.length > 0) {
    throw new Error('Bad password.');
  }

  doSomethingWithThePassword();
}

As I always work with null/undefined instead of an empty string, this was what I did intuitively (no check for an empty string). There will be no compiler error from TypeScript. So I run the code... and what I get is an unwanted runtime behavior, even though I check the length of the warnings array to see if there are warnings. Now I have to take some time and debug what happens.

Imagine it would return null instead of ''. TypeScript would complain that null is not allowed inside of that string[] and I could see instantly what the problem is.

With just string it is ambiguous, while string | null (or string | undefined) is unambiguous/explicit.
For your use-case nothing changes. You can still use if (feedback.warning) and TS will give you a string inside that if-block.
You can even get rid of that null if you really want by using strictNullChecks: false.

[MrWook]

That use case seems more than reasonable 👍

Will change it to null, which would be a breaking change but good thing i'm currently developing against a new major version anyway

[U-4-E-A]

Victory!

Can I ask what changes we will see in the new major version?

[MrWook]

Apart from this change, it will be everything since the MR #174
Which means:

• language packages treeshakeable
• removal of default exports
• networkError handler options for the pwned matcher
• diceware dictionary with separated scoring for the dictionary matcher
• a compound splitter which is used by the generator of the dictionaries => smaller bundle size for the spanish package

Additionally i'm trying to work on a "unmunger" instead of the l33t speak to search for more complex l33t speak stuff. If i don't get this done i can at least fix #118 which is kind of breaking.

Another thing that i'm working on is trying to fix the ReDOS vulnerability dropbox/zxcvbn#327

It would be nice if i could land the seperator MR too #115

[Tostino]

@MrWook I have a new PR for implementing an "unmunger" if you want to look through it: GoSimpleLLC/nbvcxz#84

I haven't had a chance to look through it to merge yet, but wanted to bring it to your attention.