mapOf and setOf validators #289

conartist6 · 2019-08-29T23:33:44Z

Fixes #190

I also intend to add iterableOf to this.

conartist6 · 2019-08-29T23:40:01Z

One of the things that I'm sure is worth discussing is the structure of the mapOf call, currently PropTypes.mapOf([keyValidator, valueValidator]). I thought it was appropriate to use an array, because this will be closely related to PropTypes.iterableOf. In fact quite likely I will delegate, making mapOf([k, v]) internally call iterableOf([k, v]). It feels internally consistent to me for these two validators to be related.

ljharb

Thanks, the tests are great!

It's not yet certain that this is something that will be added, and it's blocked on my comments below, but having the PR is helpful.

factoryWithTypeCheckers.js

conartist6 · 2019-08-30T00:20:01Z

So in order to allow validators to delegate to each other, e.g. mapOf delegating to iterableOf I'm going to need to do some further work with secret. Currently the design is that the validators themselves don't have the secret, so they can't possibly delegate. I assume the reason they don't have it is because that call site also would feed props into a custom validator, and the intent is to avoid leaking the secret that way.

EDIT: I'm dumb, the secret lives in a lib module. I can just use it.

conartist6 · 2019-08-30T18:29:38Z

I'm going to add a tupleOf validator as well since I need it to implement mapOf (partly) as iterableOf(tupleOf([keyType, valueType])).

conartist6 · 2019-09-03T20:43:16Z

@ljharb I guess I can't dismiss your review as stale, but I believe I have made the changes that you requested, including your improved method for detecting Map and Set instances.

Please don't ship this yet though. I have to finish writing the tests. I'm just waiting to do it until I have more of an idea whether you would accept these changes.

conartist6 · 2019-09-03T21:21:37Z

OK I take back what I said. All the tests are here. CHANGELOG and README are updated. When you get to looking at this, it is ready to merge from my perspective.

ljharb · 2019-09-18T19:03:11Z

CHANGELOG.md

@@ -1,3 +1,6 @@
+## 15.7.3


adding a new thing is semver-minor

Suggested change

## 15.7.3

## 15.8.0

ljharb · 2019-09-24T06:54:32Z

factoryWithTypeCheckers.js

@@ -269,6 +288,107 @@ module.exports = function(isValidElement, throwOnDirectAccess) {
    }
    return createChainableTypeChecker(validate);
  }
+
+  function createTupleOfTypeChecker(typeCheckerTuple) {
+    var argsInvalid = !Array.isArray(typeCheckerTuple) || !typeCheckerTuple.every(function (checker) { return typeof checker === 'function'});


Suggested change

var argsInvalid = !Array.isArray(typeCheckerTuple) || !typeCheckerTuple.every(function (checker) { return typeof checker === 'function'});

var argsInvalid = !Array.isArray(typeCheckerTuple) || typeCheckerTuple.some(function (checker) { return typeof checker !== 'function'});

ljharb · 2019-09-24T06:55:04Z

factoryWithTypeCheckers.js

+        return new PropTypeError('Property `' + propFullName + '` of component `' + componentName + '` has invalid PropType notation inside iterableOf.');
+      }
+      if (!(Symbol && Symbol.iterator)) {
+        return new PropTypeError('Cannot use `PropTypes.iterableOf` if Symbol.iterator is not present');


technically this isn't true; you could use Array.from exclusively, which can be polyfilled.

ljharb · 2019-09-24T06:57:21Z

factoryWithTypeCheckers.js

+      var keyChecker = typeCheckers[0];
+      var valueChecker = typeCheckers[1];
+      if (typeof Map !== 'function') {
+        return new PropTypeError('Cannot use `PropTypes.mapOf` if the Map data structure is not present');


is this necessary? if Map isn't present, isMap will fail.

ljharb · 2019-09-24T06:57:32Z

factoryWithTypeCheckers.js

+
+    function validate(props, propName, componentName, location, propFullName) {
+      if (typeof Set !== 'function') {
+        return new PropTypeError('Cannot use `PropTypes.setOf` if the Set data structure is not present');


ljharb · 2019-09-24T06:58:10Z

lib/forEach.js

@@ -0,0 +1,9 @@
+module.exports = function forEach(iterable, callback) {
+  var iterator = iterable[Symbol.iterator]();


i think this utility should use Array.from, and fall back to explicit array/string iteration

facebook-github-bot added the CLA Signed label Aug 29, 2019

ljharb requested changes Aug 29, 2019

View reviewed changes

factoryWithTypeCheckers.js Outdated Show resolved Hide resolved

factoryWithTypeCheckers.js Outdated Show resolved Hide resolved

ljharb added new validator request Requests for a new kind of validator. Semver: minor labels Aug 29, 2019

conartist6 force-pushed the master branch from 3960ce3 to 3b486e1 Compare September 3, 2019 20:39

conartist6 force-pushed the master branch 3 times, most recently from ce9f899 to 2a391b0 Compare September 3, 2019 21:16

mapOf and setOf validators

e08542d

conartist6 force-pushed the master branch from 2a391b0 to e08542d Compare September 3, 2019 21:20

conartist6 requested a review from ljharb September 18, 2019 18:18

ljharb reviewed Sep 24, 2019

View reviewed changes

ljharb marked this pull request as draft December 22, 2021 21:34

ljharb added the GH Review: needs-revision label Dec 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mapOf and setOf validators #289

mapOf and setOf validators #289

conartist6 commented Aug 29, 2019

conartist6 commented Aug 29, 2019

ljharb left a comment

conartist6 commented Aug 30, 2019 •

edited

conartist6 commented Aug 30, 2019

conartist6 commented Sep 3, 2019

conartist6 commented Sep 3, 2019

ljharb Sep 18, 2019

ljharb Sep 24, 2019

ljharb Sep 24, 2019

ljharb Sep 24, 2019

ljharb Sep 24, 2019

ljharb Sep 24, 2019

	var argsInvalid = !Array.isArray(typeCheckerTuple) \|\| !typeCheckerTuple.every(function (checker) { return typeof checker === 'function'});
	var argsInvalid = !Array.isArray(typeCheckerTuple) \|\| typeCheckerTuple.some(function (checker) { return typeof checker !== 'function'});

		@@ -0,0 +1,9 @@
		module.exports = function forEach(iterable, callback) {
		var iterator = iterable[Symbol.iterator]();

mapOf and setOf validators #289

Are you sure you want to change the base?

mapOf and setOf validators #289

Conversation

conartist6 commented Aug 29, 2019

conartist6 commented Aug 29, 2019

ljharb left a comment

Choose a reason for hiding this comment

conartist6 commented Aug 30, 2019 • edited

conartist6 commented Aug 30, 2019

conartist6 commented Sep 3, 2019

conartist6 commented Sep 3, 2019

ljharb Sep 18, 2019

Choose a reason for hiding this comment

ljharb Sep 24, 2019

Choose a reason for hiding this comment

ljharb Sep 24, 2019

Choose a reason for hiding this comment

ljharb Sep 24, 2019

Choose a reason for hiding this comment

ljharb Sep 24, 2019

Choose a reason for hiding this comment

ljharb Sep 24, 2019

Choose a reason for hiding this comment

conartist6 commented Aug 30, 2019 •

edited