Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Pimm opened this issue Jun 2, 2018 · 23 comments · May be fixed by #289
Closed

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

Pimm opened this issue Jun 2, 2018 · 23 comments · May be fixed by #289
Labels
new validator request Requests for a new kind of validator.

Comments

@Pimm
Copy link

Pimm commented Jun 2, 2018

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
Copy link
Collaborator

ljharb commented Jun 2, 2018

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 Pimm changed the title .set and .setOf .set and .setOf (and .map and .mapOf) Jun 4, 2018
@Pimm
Copy link
Author

Pimm commented Jun 4, 2018

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
Copy link
Collaborator

ljharb commented Jun 4, 2018

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

@Pimm
Copy link
Author

Pimm commented Jun 5, 2018

@ljharb That is true.

In which realm does instanceof Set not work?

Pimm added a commit to Pimm/prop-types that referenced this issue Jun 5, 2018
@ljharb
Copy link
Collaborator

ljharb commented Jun 5, 2018

@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.

@ljharb ljharb added the new validator request Requests for a new kind of validator. label Feb 21, 2019
@conartist6
Copy link
Contributor

conartist6 commented Aug 29, 2019

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
Copy link
Contributor

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
Copy link
Collaborator

ljharb commented Aug 30, 2019

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
Copy link
Collaborator

ljharb commented Aug 30, 2019

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

@endash
Copy link

endash commented Dec 6, 2020

(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
Copy link
Contributor

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

@conartist6

This comment has been minimized.

@conartist6

This comment has been minimized.

@endash

This comment has been minimized.

@conartist6

This comment has been minimized.

@ljharb

This comment has been minimized.

@conartist6

This comment has been minimized.

@ljharb

This comment has been minimized.

@endash

This comment has been minimized.

@conartist6

This comment has been minimized.

@endash

This comment has been minimized.

@conartist6

This comment has been minimized.

@ljharb
Copy link
Collaborator

ljharb commented Dec 7, 2020

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.

@ljharb ljharb closed this as completed Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new validator request Requests for a new kind of validator.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants