Implement mapOf setting type

[kimjoar]

@azasypkin This should help with the logging stuff, I expect ;)

[kimjoar]

These errors aren't fantastic at giving you context yet (e.g. this doesn't separate between if the key failed or the value failed), but I think that's a separate PR cleaning it up across the Setting stuff.

[azasypkin]

I think it's good enough for now. I still need to learn what these jest snapshot are about :)

[azasypkin]

Looks great, just few nits. I will battle test it with my logging settings :)

[azasypkin]

question: what is stopping us from using something like this?

function isMap<K, V>(o: Map<K, V>) {
  return o instanceof Map;
}

[kimjoar]

🙈 "search internet, copy first answer" didn't work :partyparrot: <--- github needs to add this asap

[azasypkin]

note: not for this PR, but maybe for the future discussion, from my experience validate usually has one of the two behaviours:

• Returns bool (is validated or not);
• Throws if can't validate.

And never modifies anything.

In this case it validates and transforms value somehow, so validate name is a bit misleading, but maybe that's just me.

[kimjoar]

I agree, I just haven't found a better name 🙈

[azasypkin]

nit: I actually find[validatedKey, validatedValue] as [K, V] more readable and it requires less code, but up to you :)

[kimjoar]

Yeah, I did that at first, but added toTuple to have a place to add a comment. But I agree, I'll change it. 👍

[azasypkin]

nit: Hmm, these two pieces look so similar, maybe we can do something like this?

let keyValuePairs;

if (isPlainObject(obj)) {
  keyValuePairs = Object.keys(obj).map((key) => [key, obj[key]]);
} else if (isMap(obj)) {
  keyValuePairs = Array.from(obj);
} else {
  throw new SettingError(
    `expected value of type [Map] or [object] but got [${typeDetect(obj)}]`,
    context
  );
}

const res = keyValuePairs.map(([key, value]) => {
  // You see, I don't like local variables if it's easy to read and TS doesn't complain :D
  // But it's personal preference, so up to you 
  return [
    this.keySetting.validate(key, toContext(context, key)),
    this.valueSetting.validate(value, toContext(context, key))
  ] as [K, V];
});

return new Map(res);

Although I think additional map call is not a big deal in this code (+ we'll get rid of it once we get Object.entries), we can go further and iterate lazily if you have performance concerns:

let keyValuePairs;

if (isPlainObject(obj)) {
  keyValuePairs = function* iterator() {
    for (const key of Object.keys(obj)) {
      yield [key, obj[key]];
    }
  }();
} else if (isMap(obj)) {
  keyValuePairs = obj;
} else {
  throw new SettingError(
    `expected value of type [Map] or [object] but got [${typeDetect(obj)}]`,
    context
  );
}

const resultMap = new Map();
for (const [key, value] of keyValuePairs) {
  resultMap.set(
    this.keySetting.validate(key, toContext(context, key)),
    this.valueSetting.validate(value, toContext(context, key))
  );
}

return resultMap;

[kimjoar]

I extracted a function instead. It felt a tad more readable in the end after playing around with it.

[azasypkin]

nit: it would suuuuuuuper-awesome if you can add proper JSDoc for this exported function (with tiny example maybe) :)

[kimjoar]

👍 I'll create a separate PR where I add a jsdoc for all of these

[azasypkin]

Nice!

[azasypkin]

I think it's good enough for now. I still need to learn what these jest snapshot are about :)