.set and .setOf (and .map and .mapOf)

[Pimm]

In a project I'm currently working on, I pass Sets to components.

As a fanatic user of prop-types to make components easier to reuse/rearrange, I write PropTypes.arrayOf(PopTypes.string.isRequired) quite often. However, there is currently no counterpart for sets. One resorts to defining a prop PropTypes.object. This alternative doesn't have quite the positive effect on readability as .arrayOf.

I would like to discuss adding .set and especially .setOf.

I wrote a proof-of-concept. I am also more than happy to provide a pull request including everything from implementation to tests, but figured the wise thing to do is ask for some early feedback.

function createSetOfTypeChecker(typeChecker) {
  function validate(props, propName, componentName, location, propFullName) {
    if (typeof typeChecker !== 'function') {
      return new PropTypeError('Property `' + propFullName + '` of component `' + componentName + '` has invalid PropType notation inside setOf.');
    }
    var propValue = props[propName];
    if (!(propValue instanceof Set)) {
      var propType = getPropType(propValue);
      return new PropTypeError('Invalid ' + location + ' `' + propFullName + '` of type ' + ('`' + propType + '` supplied to `' + componentName + '`, expected a set.'));
    }
    var insideValidateResult = null;
    var insideValidateContainer = {};
    propValue.forEach(function checkValue(value) {
      if (null !== insideValidateResult) {
        return;
      }
      insideValidateContainer.value = value;
      var error = typeChecker(insideValidateContainer, 'value', componentName, 'value', 'inside ' + propFullName, ReactPropTypesSecret);
      if (error instanceof Error) {
        insideValidateResult = error;
      }
    });
    return insideValidateResult;
  }
  return createChainableTypeChecker(validate);
}

Errors in this implementation, unfortunately, look like this: Invalid value `inside highlightedIds` of type `string` supplied to `NameList`, expected `number`.

As values inside a set are not a direct property of the set, it is more difficult to name it in the error message. If the fifth value in an array has the wrong type, the error will blab out that it's array[4]. In a set, I guess the best thing we could do is say that it's inside set.

Has this been discussed before?

[ljharb]

instanceof is unreliable, and doesn't work cross-realm - the only safe way to identify Sets is function isSet(maybeSet) { try { Object.getOwnPropertyDescriptor(Set.prototype, 'size').get.call(maybeSet); return true; } catch { return false; } }.

You can already do what you want by composing PropTypes.instanceOf(Set) with something that checks the values; it's not a bad suggestion though!

(Also, if Set was added, I'd expect Map to be added as well)

[Pimm]

Thanks for the feedback @ljharb.

.mapOf would likely require passing two checkers: one for the keys and one for the values.

Any thoughts on the ugly errors?

[ljharb]

I don’t see why it must; objectOf doesn’t take a checker for the keys, only the values.

[Pimm]

@ljharb That is true.

In which realm does instanceof Set not work?

[ljharb]

@pimm it’s not that; it’s that using a Set from an iframe will return false with instanceof. It’s the reason Array.isArray exists.

[conartist6]

I'm interested in this. I've written a PR, though I obviously encountered the issue with the array access operator. I was able to work around it. I'm also quite sure that maps should take two validators, one for keys and one for values. It doesn't make sense to have validators for a dictionary-style object as validated by objectOf since the keys are always just going to be strings. That is very much not the case with Maps.

I hadn't considered the problems with instanceof Set. Here's what I think can be done about that:

• If the instanceOf Set check succeeds, you're fine (obviously)
• If the instanceOf Set check fails, look more closely at the object. Does it have the methods you expect, namely add, has, and forEach. This check doesn't have to be perfect.
  • If you get this far but don't find Symbol.iterator, you're probably looking at an IE10 Set. Fail the check and warn about IE10.
  • If you do find Symbol.iterator keys values and entries, it's pretty certain that what you have is a Set of some description. Warn the user that it looks like they may be encountering a Set from a different realm and, this is the key bit, link to a tinyUrl page about how to fix the problem. On that page recommend that they change their setOf validator to an iterableOf and, if they need the other set methods, internally wrap with a Set which belongs to their own realm.

[conartist6]

As a note, I have a temporarily shelved project which would use symbols to allow custom structures like Immutable.Set to declare themselves to follow the Set API, and thus would be suitable for allowing sets to truly be reliably detected across realms.

[ljharb]

instanceof Set can succeed on a non-set, and can fail on a real set.

I think the check does have to be perfect.

[ljharb]

also note that arrays, Maps, and Sets all have Symbol.iterator, values, and entries.

[endash]

(Presumably objectOf doesn't take a checker for the keys because the keys of an object are guaranteed to be strings. Map keys can be anything. Edit: Strictly speaking, Symbols can also be keys, but anything else just gets turned into a string. For that matter, Symbol keys don't show up in Object.keys/.values/.entries.)

For me, I see there being two separate use cases at play:

1. I store and pass around Sets at a higher level to easily guarantee against duplicate values, but the component doesn't care that it actually gets a Set. Sometimes it might be odd if it were accidentally passed a prop with duplicate values, but the component is just doing its rendering thing, and any important business rule around duplicates is handled at the source of truth. In these cases, what I'd actually prefer is an iterableOf check for the generic interface that the component actually relies on.

2. The component uses the Set interface, usually has. A setOf type seems like it would be nice, here, except that the component doesn't actually need to get a Set to use has, it just need an iterable it can wrap in a Set.

In both cases, I always want to maintain the ease of passing in a Set, without the component necessarily having to care that it's getting a Set.

The caveat to iterableOf, which in principle would be simple to add, is specifying the shape of the values. Single values are simple enough. Map, though, is an iterable of key-value arrays, e..g ['foo', 15], as is Object.entries. There isn't a product type to match on a heterogenous array like this.

I see three options:

1. Add a top-level product-typed array checker, that matches the types of elements at a specific index of a single array. Downside: People would use this, and product-typed arrays are generally an anti-pattern. Also strictness/non-strict considerations. Also why not support iterables here too, at that point?

2. Support calling .iterableOf with either: a single iterated type; or an array of types that would be checked against the entries as if they were a product-typed array. Downside: potential weird ambiguities around .iterableOf(.string)/.iterableOf([ .string ]).

3. Explicitly limit support to list-like or map-like semantics, with either one or two types: .iterableOf(.string) or .iterableOf(.string, .number). Downside: couldn't match against iterables of product-typed arrays generically.

(Not a big deal IMO but it's worth noting that the nice array/set symmetry you get from treating them as iterables doesn't apply to object/map, since POJOs aren't, strictly speaking, iterables. But that's fine, we already have a type for POJOs.)

[conartist6]

Oh yeah you need a top level tupleOf matcher. Then you'd have iterableOf(tupleOf([keyType, valueType])) I think.

[conartist6]

I just burned out on prop-types completely a long time ago. It's pretty much an automatic reaction to rip it out of code I touch at this point. Type checkers are the way of the future, and since prop types only print warnings when they fail all it tends to get you is test output full out warnings and code that may be actively misleading its reader, which at worst will cause bugs and at best will just make people scared to change code ever. And there's no good way of knowing if you're looking at a correct prop types or incorrect ones. What do you do, run the whole test suite and look for logs generated from that line? As soon as the output is littered with prop types warnings that becomes too much work, and there's no decent way to make sure that you don't check in code that introduces new warnings.

[conartist6]

Now this could be made significantly better if there was a flag to make prop types throw. Then you could turn that on in tests and you'd at least know your checks were valid. The fact that feature hasn't been built in the past however many years, (combined with the fact that its usage curve is flat) makes me think the best thing for it now would just be to put a banner up that says it isn't maintained and people should no longer use it.

[endash]

@conartist6 Once I saw the timestamp on the last change, I figured the writing was on the wall. It's a bit of a shame, IMO, since unexpected props are such a huge source of bugs, and prop-types is so lightweight compared to a true static type system.

That said, I don't think I ever really saw prop-types used properly in the field, at least not consistently. Ultimately, a lot of what you're describing the effects of: prop types as code duplication, just reflecting the status quo of what values are being plugged in by the parents at the moment of implementation, instead of being used to declare an interface the components themselves rely on. When the prop types are being randomly driven by the set of all consumers, they get out of sync super easily and IMO are pretty meaningless to begin with.

My hunch is the reason a throwing option—let alone making that the default behavior—never happened is because of how common it is for prop-types to simply be a point-in-time description of data flows, as a form of make-work, and not actually a useful tool for avoiding bugs.

[conartist6]

Yeah I wouldn't suggest making it the default, just making it an option that people would turn on in their test configurations. But I think most people have decided the weight of the type system is worth it. The speed at which it can give feedback is usually worth the cost given the difficulty of running the correct tests in large and interconnected systems.

[ljharb]

@conartist6 there is: set your test suite to throw on any console error or warning, which I do on every test suite I touch. PropTypes are immensely useful when you have this, and use exact everywhere - far more in my experience than type systems, since PropTypes can actually check runtime values, which TS can't.

[conartist6]

@ljharb Idunno about that. I applaud the intent, but I would not do what you are describing. The problem is deprecation warnings. They don't mean that anything is broken or wrong right now, just that work will need to be done soon. Often the code that replaces deprecated code isn't ready (or isn't completely ready) at the moment a deprecation warning is added. You betcha I'm gonna be swearing loudly as my inner monologue says "I was just trying to do my job" when the build breaks because a dependency of a dependency updated and now prints a warning, which the author did not consider semver-breaking. Prop type failures on the other hand do describe something that is wrong, and I definitely want to know about it in my test environment even if I'm not committed to the level of pristine environment that would be throwing on all warnings.

[ljharb]

It’s pretty trivial to make the console mocks that throw use an exclude list, which can easily filter out messages you know are safe to ignore.

[endash]

That's sounding pretty brittle to my ears. I'm not one to make the perfect the enemy of the good, but the more test suites get away from the simplicity of exceptions and assertions causing failures the more they leak like a sieve. A warning is just that... repurposing it into an exception because you don't otherwise have a hook into the system to get it to raise an exception is the start of a rabbit hole.

[conartist6]

Pretty trivial perhaps, but I wouldn't want to start every codebase I write that way. I'd add it if it got to the level of maturity where I thought the benefit was greater than the cost. Also there's a ton of codebases that exist already that don't do that. create-react-app doesn't does it? And I can't even find a tool that provides whitelist functionality with google. So I don't think prop-types has the leverage to change how everyone works (in a way not everyone will agree with), and so by only emitting warnings most average engineers working corporate jobs end with a codebase that has warnings in the output and prop types that are wrong.

[endash]

On the flip side... the last commit to actual code was 2 years ago, so you'd probably be pretty safe just forking it and making it do whatever you wanted. The problem there is building a fork of a possibly-functionally-deprecated library further into your process...

Having spent the day on my third foray into typescript+React, a fork is looking mighty fine right now.

[conartist6]

@endash Typescript can do runtime checking of values too. Check out the ts-predicates package. It would be awesome if someone built a library that combined the power of prop-types with the safety and static analysis that ts-predicates offers. So I guess yes, I think a fork could make some leaps and bounds that would allow it to compete with this package.

[ljharb]

I don't think it's worth it to add these validators to prop-types, especially given react's general position on PropTypes as a concept. Here's what I'd suggest in the meantime:

import isSet from 'is-set';
import isMap from 'is-map';
import { predicate, and } from 'airbnb-prop-types';

const set = predicate(isSet, 'must be a Set');
const map = predicate(isMap, 'must be a Set');

cont setOf = (validator) => and([set, validator]);
const mapOf = (validator) => and([map, validator]);

You'll find .isRequired and that all expected propType debugging info is available.

General discussion about the value of PropTypes as a concept don't belong on this issue, or really on this repo at all.