Zaubrik / djwt

Create and verify JSON Web Tokens (JWT) with Deno or the browser.
MIT License
228 stars 23 forks source link

Removed the any types in algorithm.ts. #81

Closed jnssnmrcs closed 1 year ago

timonson commented 1 year ago

Thank you very much @jnssnmrcs ! Is it possible to check for the deeper name property, too?

jnssnmrcs commented 1 year ago

IMO it's enough to check for a hash property to narrow the type KeyAlgorithm to HmacKeyAlgorithm | RsaHashedKeyAlgorithm since there are no other interfaces that extends KeyAlgorithm with a property named hash. I guess you could check for the deeper property name like so:

function isHashedKeyAlgorithm(
  algorithm: KeyAlgorithm
): algorithm is HmacKeyAlgorithm | RsaHashedKeyAlgorithm {
  return (
    "hash" in algorithm &&
    isObject(algorithm.hash) &&
    "name" in algorithm.hash &&
    isString(algorithm.hash.name)
  );
}

and for the other one to verify its type:

const isEcKeyAlgorithm = (
  algorithm: KeyAlgorithm
): algorithm is EcKeyAlgorithm => {
  return "namedCurve" in algorithm && isString(algorithm.namedCurve);
};

but then it's even cleaner to just use any like this:

function isHashedKeyAlgorithm(
  algorithm: any
): algorithm is HmacKeyAlgorithm | RsaHashedKeyAlgorithm {
  return isString(algorithm?.hash?.name);
}

console.log(isHashedKeyAlgorithm("test")); // false
console.log(isHashedKeyAlgorithm(null)); // false
console.log(isHashedKeyAlgorithm(undefined)); // false
console.log(isHashedKeyAlgorithm({})); // false
console.log(isHashedKeyAlgorithm(3)); // false
console.log(isHashedKeyAlgorithm({ hash: "string" })); // false
console.log(isHashedKeyAlgorithm({ hash: { name: 34 } })); // false
console.log(isHashedKeyAlgorithm({ hash: { name: "name" } })); // true

or if you wanna restrict the input to KeyAlgorithm:

function isHashedKeyAlgorithm(
  algorithm: KeyAlgorithm
): algorithm is HmacKeyAlgorithm | RsaHashedKeyAlgorithm {
  return isString((algorithm as any)?.hash?.name);
}

I would have gone with the approach I have committed since as I said, it's enough to narrow the type. If they in the future change it or adds another type with a hash property you would have to update this code, but you would have to do that either way.

timonson commented 1 year ago

Thanks! I will merge it after I took a second look! Are you OK with it? I am able to reopen this PR, I think.

jnssnmrcs commented 1 year ago

Sure!