Discussion about props validation

[mcjazzyfunky]

The following is just meant to be (hopefully) base of a discussion - not a feature request yet.

DIO does not provide a feature to validate the component props (known as "propTypes" in react-development).
I really can understand that this is not implemented in DIO.
Nevertheless - especially if you do not use TypeScript - the development experience would be highly increased if you had some kind of props validation in your development environment.
Of course programmers can write their own Component extension that adds that validation if they want - using super constructor and componentWillReceiveProps to add the props validation features.
But whatever they do, it will not result in the same programming experience that they would have with React (there will be some annoying additional boilerblate code and error messages will not show the path within the component tree etc.).

Frankly, I personally do not like the way the whole component metadata has to be declared in React, especially the propsType API - what about the following simple non-React-conform solution:

DIO's Component class povides a static class methods validateProps(props: PropsType): Error | Error[] | null (the base implementation returns null) that will be called before each constructor call and before each invocation of componentWillReceiveProps. If it returns an error or an array of errors those errors will be printed out on console using console.error or console.warn (whatever you like) on DEV, including information about the concreate node in the component tree =>that's all.
In PRODUCTION the validateProps class method will just be ignored.

As static class methods will be inherited to subclasses and the corresponding (sub)class (aka. constructor) can be accessed via this inside of a static class method, all programmers could (optionally) implement/use the validation system of their choice.

Oh BTW: Since a couple of weeks, React's context providers can also have prop validation. That also should be added as suggested above.

[thysultan]

Currently all shipped artifacts are meant for production builds so we'd first need to introduce the concept of a development build. I'm happy to review a PR that sets this up.

What did you not like about the React propsType API specifically

[aMarCruz]

I think propTypes will be deprecated in future versions of React, and with the use of Typescript or Flow this does not seem to have a greater impact.

[mcjazzyfunky]

@aMarCruz "propTypes will be deprecated in future versions of React" - where does this information come from?
Typescript and Flow can for example not check wheter a number is an integer, whether a string matches a special regex, whether a float value is between 0 an 100 etc.
With propTypes that's possible, of course.
Moreover, if you implement a component library which shall also be usable with ECMAScript directly, Typescript's and Flow's compile time type checking will not always be a big help regarding props validation.

@thysultan My comment above that I personally do not like the propType API was completely unnecessary and is not important for this suggestion/discussion, sorry. Nevertheless, if someone is interested in my humble opinion, the main reason why I do not like it that much, is because the propTypes library is completely React-specific. I would prefer a general and React independent data validation library that can also be used for all the other validation purposes to be found in each application besides UI component properties.
Absolutely not important but who is still interested:
I personally use this library here for prop validations in React: https://github.com/js-works/js-spec => sorry still alpha, no real README - you have to look into the source code, yet working and containing unit tests - of course you need some adapter function or class decorator to use it with React.

By the end of the day I think there are three reasonable options:

Option 1: DIO stays as is and will not provide any support for prop validation.

Option 2: DIO will implement the propTypes API like predefied by React

Option 3: DIO will provide just minimalistic and therefore very basic support for prop validation. Implementation details are completely up to the component developer. => This is what I have suggested above.

[Edit - removed final comment about personal preference, as this is not important]

[aMarCruz]

@mcjazzyfunky sorry, I got confused because of an article on the React blog. React continues to provide support through the external "prop-types" package.

Anyway, the reasons they gave for removing propTypes from the core seems right to me, Typescript for example, with a correct setup, offers static analysis at a level that propTypes can not do, without affecting performance at runtime.

In my Apps I used Typescript from the beginning and I have never found any error in components that TS has not detected with the correct typings).

Of course, the propTypes API does not hurt (I like the defaultProps feature) and I know that it is very useful for those who do not use any tool like the ones I mentioned.

EDIT: ...and I agree that the cases you mention (regex, ranges, etc) are the best of propTypes.

[thysultan]

Supporting the propTypes API could be:

if ('production' === ENVIROMENT) {
	if (type.propTypes) {
		for (var [name, fn] of type.propTypes)
			fn(name, props, type, ....)
	}
}

Ergo option 2, this however is predicated on introducing this to the build system to support producing production and development builds that strip this out of production so that this doesn't leek into production builds(the default main entry).

@mcjazzyfunky If you're up to this i can help or review any PR's with this in mind.

[mcjazzyfunky]

@thysultan Frankly, I'm a bit busy currently, but if it's okay for you then we close this discussion issue here and I will provide a PR for that DEV/PROD split in the DIO build scripts in a couple of weeks by opening a new issue for that PR.
This prop validation feature is not really urgent - so it is surely not a big deal if I start with that build script changes sometime in August, okay?

[mcjazzyfunky]

Some additional information:

React has a dependency on 'prop-types':
https://github.com/facebook/react/blob/82c7ca4cca90976cd1230e03ea1a4372a4276e67/packages/react/package.json#L26

This is because it needs to call the 'prop-types' function 'checkPropTypes':
https://github.com/facebook/react/blob/f9358c51c8de93abe3cdd0f4720b489befad8c48/packages/react/src/ReactElementValidator.js#L23

That's perhaps mainly because of the last argument for the propType validation functions must be 'SECRET_DO_NOT_PASS_THIS_OR_YOU_WILL_BE_FIRED' which is obviously not part of the public API => https://github.com/facebook/prop-types/blob/master/lib/ReactPropTypesSecret.js

This is a bit unpleasant, because it means that DIO would also need package 'prop-types' as a dependency (currently DIO has no dependencies).
The only workaround to prevent that dependency seems to be to hardcode that 'SECRET_DO_NOT_PASS_THIS_OR_YOU_WILL_BE_FIRED' argument, which obviously would be really, really ugly.

Any opinions/comments/ideas regarding this?

[mcjazzyfunky]

A further problem:

Package 'prop-types' has the word "React" hard coded in its error messages:
https://github.com/facebook/prop-types/blob/master/checkPropTypes.js#L66

[thysultan]

Sure, whenever you are able.

An aside, by option 2 i didn't mean supporting the prop-types package specifically, just supporting the ability to define propTypes since the prop-types package like you said is intentionally React specific given that it doesn't support generic function calls on the validators.

[mcjazzyfunky]

Oh, stupid me: I've seen that 'prop-types' has only a dev dependency on React (for the unit tests), so I mistakenly thought that it may be possible to wrap the 'prop-types' package to be used with DIO.
But of course it needs some UI library specific isValidElement function which is re-implemented here:
https://github.com/facebook/prop-types/blob/master/index.js#L14

Means, some day there really should be a "dio-prop-types" library - which may be a DIO adapted fork of the original 'prop-types' library.

[mcjazzyfunky]

Result of the discussion (Edit: hope my interpretation is correct):

1. @mcjazzyfunky will provide a PR which will extend the scripts in folder "script/build" to produce both "development" and "production" versions of each package => he will open a new issue then.
2. @thysultan will review and test the above mentioned PR by @mcjazzyfunky.
3. If 2. is finished @thysultan or @mcjazzyfunky (or someone else - tbd) will implement support for propTypes for Component classes and context providers with the same API like React. This implementation will not have any dependencies on other packages.

[Zolmeister]

Does this mean DIO will be responsible for maintaining a functionally-equivalent version of the prop-types package in the core build?

I believe that proposal goes too far. Is there no middle ground for a modular approach?

[mcjazzyfunky]

@Zolmeister Okay, maybe I was a bit too fast by closing the discussion. I will reopen this issue. Zoli, do you have a proposal/idea/opinion regarding the prop validation?

[mcjazzyfunky]

@Zolmeister DIO's core will not contain a "functionally-equivalent version of the prop-types package" but only a more or less similar logic to the checkPropTypes function of prop-types.
Nevertheless, indeed, it needs some external library with some useful validation functions to actually benefit from this new feature.

[Zolmeister]

I see two competing cases.

1. I am using a React component (prop-types must match React) so that when I interface I get nice debug output
   1. requires functionally-equivalent handling (otherwise mixing DIO/React components will break)
2. I want to validation my own/DIO components

For the latter I'd simply validate in render:

const config = require('../../config')
const Joi = require('joi')
class Abc {
   static propTypes = {
     a: Joi.string(),
     b: Joi.number(),
     c: Joi.string().regex(/[a-z0-9]/)
   }
   render({a, b, c}) {
     config.ASSERT_PROPS && Joi.assert(this.props, Abc.propTypes)
   }
}

I'm also having trouble seeing the immediate benefit of a separate development build of the library
I'd like to avoid a separate development build

Edit: if you're after a DIO standard validation syntax (e.g. for components from libraries), perhaps a simple global flag on the dio singleton would suffice

[thysultan]

@mcjazzyfunky The above mentioned code in #issuecomment-406704827 would more or less suffice, the checkPropTypes is after all best left to the external prop-types package that implements this validation logic, DIO would just need to call whatever functions it finds in the propTypes object with fn(element, type, props, name), from that everything listed in checkPropTypes.js#L41 can be derived externally.

@Zolmeister Unrelated to this, development builds might have more detailed error messages in certain cases.

However unlike older versions of React the main entry should be the production build that is to say you need to manually opt to a dev build like you do for the esm build.

[mcjazzyfunky]

@thysultan I would try to implement this "propTypes" thingy if nobody else has time to do so (would open an extra issue for that task).
Please let me know if you prefer to implement this on your own.

[Edit: Removed comments about "new" vs "legacy" context API, as this is not important here]
As for the "legacy" context API I would suggest to implement support for "contextTypes" but not to implement support for "childContextTypes" (as I do not really think the latter would make great sense in DIO).

[thysultan]

@mcjazzyfunky Sure, you can pick this up, and i agree childContextTypes doesn't make sense in DIO.