API for checking external objects against React prop types #9004

acdlite · 2017-02-14T22:56:21Z

Allows users to check arbitrary objects against React prop types without relying on the implementation details of the prop validators themselves. An example of why this is important: some prop validators may throw, while others return an error object. Calling a prop validator manually requires handling both cases. checkPropTypes does this for you.

The default behavior is to skip repeated warnings, like normal prop type checking already does. I added an optional parameter to override that and always warn; not sure if we really need that though.

gaearon · 2017-02-15T00:58:55Z

src/isomorphic/classic/__tests__/ReactContextValidator-test.js

@@ -261,7 +261,7 @@ describe('ReactContextValidator', () => {
    ReactTestUtils.renderIntoDocument(<Component testContext={{bar: 123}} />);
    expectDev(console.error.calls.count()).toBe(1);
    expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe(
-      'Warning: Failed childContext type: ' +
+      'Warning: Failed child context type: ' +


If this is released in 15.x, it would potentially break CIs on libraries that assert about this warning in tests.

We have some tests like this about prop types in React Redux so I wouldn't be surprised if somebody tested a similar thing about context types.

I don't think this needs to be released until 16. Could be mistaken.

acdlite · 2017-02-15T01:02:18Z

Also I don't have a a strong opinion on the top-level API. It is a bit weird that checkPropTypes is the only member of React.PropTypes that is not a prop type. Open to other API suggestions, just chose this one because it was the most straightforward for now.

bvaughn · 2017-02-15T17:26:41Z

It is a bit weird that checkPropTypes is the only member of React.PropTypes that is not a prop type.

Agreed it's a bit weird. Don't feel super strongly about it though.

function checkPropTypes(propTypes, object, location, componentName, warnOnRepeat) {}

Something about the number and ordering of arguments here feels a bit awkward to me from an external use perspective. (Although I haven't found myself wanting to manually validate propTypes before so perhaps I'm just not a good representative for this use case.)

I thought a bit about reducing the number of parameters required by the external interface by creating a couple of wrapper functions (eg checkContextTypes(contextTypes, context, componentName = 'React class')) but this feels awkward too. Maybe it's okay as-is.

Sorry for the less than helpful feedback 😄

aweary · 2017-02-15T17:59:25Z

Whats the use case for manually checking an object against propTypes? Won't this just encourage and provide an API for users to couple application logic to propType validation, which is supposed to be stripped in production?

sebmarkbage · 2017-02-15T18:32:03Z

@aweary It used to be the case that people were doing this anyway. They were sloppy about making it DEV only, so they could rely on it working in production, which is why we've turned all these into noops in production. Since this method doesn't throw or return a value, there's nothing to rely on.

sebmarkbage · 2017-02-15T18:35:49Z

src/isomorphic/classic/types/ReactPropTypes.js

@@ -123,6 +123,12 @@ if (__DEV__) {
  };
 }

+function checkPropTypes(propTypes, object, location, componentName, warnOnRepeat) {
+  return checkReactTypeSpec(propTypes, object, location, componentName, null, null, warnOnRepeat);


Can you remove the return here?

Can you confirm that this never returns a value and never throws? That is important because that is what lets us strip it out safely in production.

sebmarkbage · 2017-02-15T18:42:06Z

Ideally we should be able to express all our validation logic in terms of this helper. That way we can move this to an external reusable library and just call into it from ReactElementValidator directly. I.e. invert the dependency between checkReactTypeSpec and checkPropTypes. Can we describe checkReactTypeSpec in terms of checkPropTypes instead of vice versa?

What would we need from the API to make it possible?

Perhaps we need to pass a callback into this helper to provide further formatting options? That might mean that we don't need as many arguments passed.

acdlite · 2017-02-16T00:12:15Z

Ok I inverted the dependency between checkPropTypes and checkReactTypeSpec.

I wasn't able to cut down the number of arguments much. In order to do that, we'd need to change the signature of the validators themselves, which currently expect componentName and location.

sebmarkbage · 2017-02-16T00:49:41Z

src/shared/types/checkReactTypeSpec.js

-        error = typeSpecs[typeSpecName](values, typeSpecName, componentName, location, null, ReactPropTypesSecret);
-      } catch (ex) {
-        error = ex;
+  function formatMessage(message) {


This closure is going to be created for every element on the entire page, and every update. That's an awful lot of allocation. Even for DEV.

I think this could be designed with global context. In Fiber we went with an approach that picked up the current context from a global mutable.

react/src/renderers/dom/fiber/wrappers/ReactDOMFiberTextarea.js

Line 72 in e36b38c

getCurrentFiberOwnerName()

That way we don't have to pass it along as an additional argument everywhere.

If it did it that way this function could just be a global instead of creating a closure.

It seems natural for things like ReactElementValidator anyway since we read the owner from ReactCurrentOwner anyway. We just pass it in a very round about way.

sebmarkbage · 2017-02-16T00:58:06Z

src/isomorphic/classic/types/checkPropTypes.js

+ * @param {?boolean} warnOnRepeat Whether or not repeated warnings should be skipped.
+ * @private
+ */
+function checkPropTypes(typeSpecs, values, location, componentName, formatMessage, warnOnRepeat) {


When do we ever want warnOnRepeat to be true? Seems like this doesn't have to be optional and simplifies the API a bit. Tests can be written by resetting the module cache.

Seemed like it might be a useful opt-in, perhaps if a lib's error messages are less specific and don't include the component name. But yeah I'll just get rid of it.

sebmarkbage · 2017-02-16T01:10:46Z

src/isomorphic/classic/types/checkPropTypes.js

+        // same error.
+        loggedTypeFailures[error.message] = true;
+
+        var message = formatMessage ? formatMessage(error.message) : error.message;


It seems like all we really need format message for is to get the stack. That seems like something very specific that would be a common use case. Maybe we should make it getStack instead?

var stack = getStack ? getStack() : ''; var message = error.message + (stack ? '\n' + stack : '');

That way, if browser devtools let us actually mess around with the stack that console.error produces, or if we can attach custom stacks to new Error, then we have an ability to do that separately.

Not sure I understand the advantage of getStack over formatMessage? Isn't the latter a superset of the former?

Not if the error logger can do something special with it in the future.

Imagine if console.error was enhanced to do something like this:

console.error({ message, stack });

Then the message was shown in the console and then the stack expanded when you click the little nub. With links to the source code.

The data structure of Error has a separate message and stack.

Similarly, if you log the warnings to a database, like we do, you likely want to put the stack in a separate column so that you can group on the error messages. If you wanted to do that with a single message, you'd have to write a manual parser that splits them back up. If every user of this API does it slightly differently, it is a pain to find a parser that can figure it out for all.

Interesting!

sebmarkbage · 2017-02-16T18:53:16Z

src/shared/types/checkReactTypeSpec.js

-          'Failed %s type: %s%s',
-          location,
-          error.message,
-          componentStackInfo


At FB we actually split each of these into a separate column in the database logs. So if we can preserve that, that'd be great.

sebmarkbage · 2017-02-22T21:07:00Z

src/isomorphic/classic/element/ReactDebugCurrentFrame.js

+  } = require('ReactComponentTreeHook');
+}
+
+const ReactDebugCurrentFrame = {


Seems like this whole thing should be wrapper in __DEV__.

sebmarkbage · 2017-02-22T21:13:14Z

src/isomorphic/classic/types/checkPropTypes.js

+ * @param {?Function} getStack Returns the component stack.
+ * @private
+ */
+function checkPropTypes(typeSpecs, values, location, componentName, getStack) {


The content of this needs to be wrapped in __DEV__ so that it can be filtered out. It won't be DCE since it's exported as a public API.

sebmarkbage · 2017-02-22T21:14:22Z

src/isomorphic/classic/types/checkPropTypes.js

+        // same error.
+        loggedTypeFailures[error.message] = true;
+
+        var stack = getStack ? getStack() : '';


Maybe default could be new Error().stack if you don't provide one? EDIT: I guess in Chrome you get that anyway.

sebmarkbage

lgtm but you need to wrap some stuff in __DEV__. Try running the build in master and then against your branch to validate that prod build size hasn't gone up.

You also need to fix the browserify stuff so that CI doesn't fail.

acdlite · 2017-02-22T23:14:05Z

   raw     gz Compared to master @ 5f6f3277f5ada837fd6096b8ad330f4914ae6200
     =      = build/react-dom-fiber.min.js
     =      = build/react-dom-server.min.js
     =      = build/react-dom.min.js
   -34    +14 build/react-with-addons.min.js
   -34     +4 build/react.min.js

ReactPropTypeLocationNames was a map of location identifiers to location display names, for use in warnings. But in practice, this did nothing except rename "childContext" to "child context." Not worth it, IMO. Since we are going to add an API for running prop type checks manually, we need the ability to support arbitrary prop type locations. So each place that used to accept a ReactPropTypeLocation now just accepts a string.

Allows users to check arbitrary objects against React prop types without relying on the implementation details of the prop validators themselves. An example of why this is important: some prop validators may throw, while others return an error object. Calling a prop validator manually requires handling both cases. `checkPropTypes` does this for you.

Expressing checkReactTypeSpec using the public API frees us to move it to a separate module in the future.

Don't need this internally; doubt external users will need it either

Gives us flexibility in the future to deal with the stack and the message separately.

There's a lot of overlap between ReactCurrentOwner, ReactDebugCurrentFrame, and ReactDebugCurrentFiber that should be consolidated.

ReactDebugCurrentFrame is shared state. checkPropTypes should be imported via the main React export, not imported directly.

facebook-github-bot added CLA Signed GH Review: review-needed labels Feb 14, 2017

acdlite changed the title ~~[WIP] API for checking external objects against React prop types~~ API for checking external objects against React prop types Feb 15, 2017

acdlite requested review from sophiebits, bvaughn, sebmarkbage and gaearon February 15, 2017 00:56

gaearon reviewed Feb 15, 2017

View reviewed changes

sebmarkbage reviewed Feb 15, 2017

View reviewed changes

acdlite force-pushed the proptypescheck branch from fb8c861 to a043ebb Compare February 15, 2017 22:10

sebmarkbage reviewed Feb 16, 2017

View reviewed changes

sebmarkbage mentioned this pull request Feb 20, 2017

React PropTypes warning with LayoutAnimation facebook/react-native#12458

Closed

acdlite force-pushed the proptypescheck branch from 9c5bd00 to 033fa98 Compare February 21, 2017 21:54

sebmarkbage mentioned this pull request Feb 21, 2017

[Fiber] Umbrella for remaining features / bugs #7925

Closed

sebmarkbage reviewed Feb 22, 2017

View reviewed changes

sebmarkbage approved these changes Feb 22, 2017

View reviewed changes

acdlite force-pushed the proptypescheck branch from 033fa98 to d115f88 Compare February 22, 2017 23:13

acdlite added the semver-major label Feb 22, 2017

acdlite removed the GH Review: review-needed label Feb 22, 2017

acdlite added this to the 16.0 milestone Feb 22, 2017

acdlite added 9 commits February 23, 2017 11:14

Test that checkPropTypes does not throw or return a value

e4acd12

Invert dependency between checkPropTypes and checkReactTypeSpec

3746a9d

Expressing checkReactTypeSpec using the public API frees us to move it to a separate module in the future.

Remove warnOnRepeat option

6ff5211

Don't need this internally; doubt external users will need it either

Change formatMessage parameter to getStack

9993fdd

Gives us flexibility in the future to deal with the stack and the message separately.

Remove closure in checkReactTypeSpec by tracking stack info globally

e6d6ad3

There's a lot of overlap between ReactCurrentOwner, ReactDebugCurrentFrame, and ReactDebugCurrentFiber that should be consolidated.

Wrap checkPropTypes in DEV conditional so it's stripped out in prod

0320b8e

Fix build config

03ed343

ReactDebugCurrentFrame is shared state. checkPropTypes should be imported via the main React export, not imported directly.

acdlite force-pushed the proptypescheck branch from d115f88 to 03ed343 Compare February 23, 2017 19:14

acdlite merged commit f3c2d9f into facebook:master Feb 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API for checking external objects against React prop types #9004

API for checking external objects against React prop types #9004

acdlite commented Feb 14, 2017 •

edited

Loading

gaearon Feb 15, 2017

acdlite Feb 15, 2017

acdlite commented Feb 15, 2017 •

edited

Loading

bvaughn commented Feb 15, 2017

aweary commented Feb 15, 2017

sebmarkbage commented Feb 15, 2017 •

edited

Loading

sebmarkbage Feb 15, 2017

sebmarkbage commented Feb 15, 2017 •

edited

Loading

acdlite commented Feb 16, 2017

sebmarkbage Feb 16, 2017 •

edited

Loading

acdlite Feb 16, 2017

sebmarkbage Feb 16, 2017

sebmarkbage Feb 16, 2017 •

edited

Loading

acdlite Feb 16, 2017

sebmarkbage Feb 16, 2017

acdlite Feb 16, 2017

sebmarkbage Feb 16, 2017

acdlite Feb 16, 2017

sebmarkbage Feb 16, 2017

sebmarkbage Feb 22, 2017

sebmarkbage Feb 22, 2017

sebmarkbage Feb 22, 2017 •

edited

Loading

sebmarkbage left a comment •

edited

Loading

acdlite commented Feb 22, 2017

API for checking external objects against React prop types #9004

API for checking external objects against React prop types #9004

Conversation

acdlite commented Feb 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite commented Feb 15, 2017 • edited Loading

bvaughn commented Feb 15, 2017

aweary commented Feb 15, 2017

sebmarkbage commented Feb 15, 2017 • edited Loading

Choose a reason for hiding this comment

sebmarkbage commented Feb 15, 2017 • edited Loading

acdlite commented Feb 16, 2017

sebmarkbage Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmarkbage Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmarkbage Feb 22, 2017 • edited Loading

Choose a reason for hiding this comment

sebmarkbage left a comment • edited Loading

Choose a reason for hiding this comment

acdlite commented Feb 22, 2017

acdlite commented Feb 14, 2017 •

edited

Loading

acdlite commented Feb 15, 2017 •

edited

Loading

sebmarkbage commented Feb 15, 2017 •

edited

Loading

sebmarkbage commented Feb 15, 2017 •

edited

Loading

sebmarkbage Feb 16, 2017 •

edited

Loading

sebmarkbage Feb 16, 2017 •

edited

Loading

sebmarkbage Feb 22, 2017 •

edited

Loading

sebmarkbage left a comment •

edited

Loading