Skip to content
This repository has been archived by the owner on Dec 16, 2021. It is now read-only.

Only run proptypes checks on element creation like React. #370

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Only run proptypes checks on element creation like React. #370

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c939f30
Merge remote-tracking branch 'developit/master'
Apr 25, 2017
177d9fc
check proptypes only on element creation like react.
Apr 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 30 additions & 17 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,10 +270,35 @@ function statelessComponentHook(Ctor) {
return Wrapped;
}

function validatePropTypes (vnode) {
if (DEV) {
let componentClass = typeof vnode.nodeName === "function"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can just always look at vnode.nodeName, right?

? vnode.nodeName
: vnode.type;

if (typeof componentClass !== "function") return;
let name = componentClass.displayName || componentClass.name;
let propTypes = componentClass.propTypes;
if (propTypes) {
let props = vnode.props;
if (
!(vnode.props && vnode.props.children) &&
vnode.children &&
vnode.children.length
) {
props.children = vnode.children;
}
propsHook(props);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought: does this need to be called even if DEV=false?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is actually called all the time. see here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I see what you mean now. 🤔 I don't think so. I could have sworn we fixed this a while back but I can't find where.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not an issue. We are rearranging the props so that prop-types detects the children correctly.

PropTypes.checkPropTypes(propTypes, props, 'prop', name);
}
}
}

function createElement(...args) {
upgradeToVNodes(args, 2);
return normalizeVNode(h(...args));
let node = h(...args);
validatePropTypes(node);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just had a thought (I think I was noodling on this when I failed to review before):

What if we moved the invocation of this validation into handleComponentVNode()? That would make proptype validation work in non-compat components if you install compat, which is kinda compelling. Also it would nicely reuse the existing logic around detecting and forking for component VNodes vs element VNodes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly got this working in a very specific way and I don't remember all the details. I do remember that I had good reasons for doing it the way I did though 😅

return normalizeVNode(node);
}


Expand All @@ -293,7 +318,6 @@ function normalizeVNode(vnode) {
}

applyEventNormalization(vnode);

return vnode;
}

Expand All @@ -315,10 +339,11 @@ function cloneElement(element, props, ...children) {
else if (props && props.children) {
cloneArgs.push(props.children);
}
return normalizeVNode(preactCloneElement(...cloneArgs));
let newNode = preactCloneElement(...cloneArgs);
validatePropTypes(newNode);
return normalizeVNode(newNode);
}


function isValidElement(element) {
return element && ((element instanceof VNode) || element.$$typeof===REACT_ELEMENT_TYPE);
}
Expand Down Expand Up @@ -489,7 +514,7 @@ function multihook(hooks, skipDuplicates) {


function newComponentHook(props, context) {
propsHook.call(this, props, context);
propsHook(props, context);
this.componentWillReceiveProps = multihook([propsHook, this.componentWillReceiveProps || 'componentWillReceiveProps']);
this.render = multihook([propsHook, beforeRender, this.render || 'render', afterRender]);
}
Expand All @@ -509,20 +534,8 @@ function propsHook(props, context) {
props.children[0] = props.children;
}
}

// add proptype checking
if (DEV) {
let ctor = typeof this==='function' ? this : this.constructor,
propTypes = this.propTypes || ctor.propTypes;
const displayName = this.displayName || ctor.name;

if (propTypes) {
PropTypes.checkPropTypes(propTypes, props, 'prop', displayName);
}
}
}


function beforeRender(props) {
currentComponent = this;
}
Expand Down
8 changes: 8 additions & 0 deletions test/component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,14 @@ describe('components', () => {
'Warning: Failed prop type: Invalid prop `bool` of type `string` supplied to `' + name + '`, expected `boolean`.'
);

console.error.reset();

const clone = React.cloneElement(<Foo func={'funky'} bool/>);
React.render(clone, scratch);
expect(console.error).to.have.been.calledWithMatch(
'Warning: Failed prop type: Invalid prop `func` of type `string` supplied to `' + name + '`, expected `function`.'
);

console.error.restore();
}

Expand Down