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

Commit

Permalink
Proposal to forward propTypes and defaultProps, see #120, #142
Browse files Browse the repository at this point in the history 
This change forward writes (but not reads!) of `propTypes` and `defaultProps` on an inject based HoC to the wrapped component, so that propTypes and defaultProps can be defined in a straight forward way.
  • Loading branch information
@mweststrate
mweststrate committed Nov 9, 2016
1 parent 7243a9d commit 666577b
Show file tree
Hide file tree
Showing 4 changed files with 116 additions and 53 deletions.
3 changes: 2 additions & 1 deletion .gitignore
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@ custom.js
custom.d.ts
index.js
index.d.ts
index.min.js
index.min.js
.vscode
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ const UserNameDisplayer = ({ user }) => (

#### Other improvements

* It is now possible to directly define `propTypes` and `defaultProps` on components wrapped with `inject` (or `observer(["stores"])`), see #120, #142. This is implemented by forwarding _writes_ to these attributes to the `wrappedComponent`, instead of applying it on the HoC itself.
* Clean up data subscriptions if an error is thrown by an `observer` component, see [#134](/~https://github.com/mobxjs/mobx-react/pull/134) by @andykog
* Print warning when `inject` and `observer` are used in the wrong order, see #146, by @delaetthomas
* export `PropTypes` as well, fixes #153
Expand Down
78 changes: 47 additions & 31 deletions src/inject.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,56 @@ import React, { PropTypes } from 'react';
import hoistStatics from 'hoist-non-react-statics';
import {observer} from './observer';

const injectorContextTypes = {
mobxStores: PropTypes.object
};
Object.seal(injectorContextTypes);

const proxiedInjectorProps = {
contextTypes: {
get: function () {
return injectorContextTypes;
},
set: function (_) {
console.warn("Mobx Injector: you are trying to attach `contextTypes` on an component decorated with `inject` (or `observer`) HOC. Please specify the contextTypes on the wrapped component instead. It is accessible through the `wrappedComponent`");
}
},
propTypes: {
set: function (value) {
this.wrappedComponent.propTypes = value;
},
get: function () {
// MWE: or is this confusing? we want the propTypes to be defined on wrapped comp, but avoid React to check it on injected?
return null;
}
},
defaultProps: {
set: function (value) {
this.wrappedComponent.defaultProps = value;
},
get: function () {
return null; // see above
}
},
isInjector: {
value: true
}
};

/**
* Store Injection
*/
function createStoreInjector(grabStoresFn, component) {
const Injector = React.createClass({
displayName: "MobXStoreInjector",
render: function() {
render: function () {
let newProps = {};
for (let key in this.props) if (this.props.hasOwnProperty(key)) {
newProps[key] = this.props[key];
}
var additionalProps = grabStoresFn(this.context.mobxStores || {}, newProps, this.context) || {};
for (let key in additionalProps) {
newProps[key] = additionalProps[key];
newProps[key] = additionalProps[key];
}
newProps.ref = instance => {
this.wrappedInstance = instance;
Expand All @@ -25,38 +61,19 @@ function createStoreInjector(grabStoresFn, component) {
}
});

Injector.isInjector = true;
Injector.contextTypes = { mobxStores: PropTypes.object };
Injector.wrappedComponent = component;
injectStaticWarnings(Injector, component)
// Static fields from component should be visible on the generated Injector
hoistStatics(Injector, component);
return Injector;
}

function injectStaticWarnings(hoc, component) {
if (typeof process === "undefined" || !process.env || process.env.NODE_ENV === "production")
return;
Injector.wrappedComponent = component;
Object.defineProperties(Injector, proxiedInjectorProps);

['propTypes', 'defaultProps', 'contextTypes'].forEach(function (prop) {
const propValue = hoc[prop];
Object.defineProperty(hoc, prop, {
set: function (_) {
// enable for testing:
var name = component.displayName || component.name;
console.warn('Mobx Injector: you are trying to attach ' + prop +
' to HOC instead of ' + name + '. Use `wrappedComponent` property.');
},
get: function () {
return propValue;
},
configurable: true
});
});
return Injector;
}


function grabStoresByName(storeNames) {
return function(baseStores, nextProps) {
storeNames.forEach(function(storeName) {
return function (baseStores, nextProps) {
storeNames.forEach(function (storeName) {
if (storeName in nextProps) // prefer props over stores
return;
if (!(storeName in baseStores))
Expand All @@ -77,7 +94,7 @@ export default function inject(/* fn(stores, nextProps) or ...storeNames */) {
let grabStoresFn;
if (typeof arguments[0] === "function") {
grabStoresFn = arguments[0];
return function(componentClass) {
return function (componentClass) {
// mark the Injector as observer, to make it react to expressions in `grabStoresFn`,
// see #111
return observer(createStoreInjector(grabStoresFn, componentClass));
Expand All @@ -87,9 +104,8 @@ export default function inject(/* fn(stores, nextProps) or ...storeNames */) {
for (let i = 0; i < arguments.length; i++)
storesNames[i] = arguments[i];
grabStoresFn = grabStoresByName(storesNames);
return function(componentClass) {
return function (componentClass) {
return createStoreInjector(grabStoresFn, componentClass);
};
}
}

87 changes: 66 additions & 21 deletions test/inject.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -256,72 +256,117 @@ test('inject based context', t => {
t.end();
})

test('warning is printed when attaching propTypes/defaultProps/contextTypes to HOC not in production', t => {
test('warning is printed when attaching contextTypes to HOC', t => {
var msg = [];
var baseWarn = console.warn;
console.warn = (m) => msg.push(m);

var C = observer(["foo"], React.createClass({
var C = inject(["foo"])(React.createClass({
displayName: 'C',
render: function () {
render: function() {
return e("div", {}, "context:" + this.props.foo);
}
}));

C.propTypes = {};
C.defaultProps = {};
C.contextTypes = {};

var B = React.createClass({
render: function () {
render: function() {
return e(C, {});
}
});

var A = React.createClass({
render: function () {
render: function() {
return e(Provider, { foo: "bar" }, e(B, {}))
}
})

const wrapper = mount(e(A));
t.equal(msg.length, 3);
t.equal(msg[0], "Mobx Injector: you are trying to attach propTypes to HOC instead of C. Use `wrappedComponent` property.");
t.equal(msg[1], "Mobx Injector: you are trying to attach defaultProps to HOC instead of C. Use `wrappedComponent` property.");
t.equal(msg[2], "Mobx Injector: you are trying to attach contextTypes to HOC instead of C. Use `wrappedComponent` property.");
t.equal(msg.length, 1);
t.equal(msg[0], "Mobx Injector: you are trying to attach `contextTypes` on an component decorated with `inject` (or `observer`) HOC. Please specify the contextTypes on the wrapped component instead. It is accessible through the `wrappedComponent`");

console.warn = baseWarn;
t.end();
})


test('warning is not printed when attaching propTypes to wrapped component', t => {
test('propTypes and defaultProps are forwarded', t => {
var msg = [];
var baseWarn = console.warn;
console.warn = (m) => msg = m;
var baseError = console.error;
console.error = (m) => msg.push(m);

var C = observer(["foo"], React.createClass({
var C = inject(["foo"])(React.createClass({
displayName: 'C',
render: function () {
return e("div", {}, "context:" + this.props.foo);
render: function() {
t.equal(this.props.y, 3);
t.equal(this.props.x, undefined);
return null;
}
}));

C.wrappedComponent.propTypes = {};
C.propTypes = {
x: React.PropTypes.func.isRequired,
z: React.PropTypes.string.isRequired,
};
C.defaultProps = {
y: 3
};

var B = React.createClass({
render: function () {
return e(C, {});
render: function() {
return e(C, { z: "test" });
}
});

var A = React.createClass({
render: function () {
render: function() {
return e(Provider, { foo: "bar" }, e(B, {}))
}
})

const wrapper = mount(e(A));
t.equal(msg.length, 1);
t.equal(msg[0].split("\n")[0], 'Warning: Failed prop type: Required prop `x` was not specified in `C`.');

console.error = baseError;
t.end();
})


test('warning is not printed when attaching propTypes to injected component', t => {
var msg = [];
var baseWarn = console.warn;
console.warn = (m) => msg = m;

var C = inject(["foo"])(React.createClass({
displayName: 'C',
render: function() {
return e("div", {}, "context:" + this.props.foo);
}
}));

C.propTypes = {};

t.equal(msg.length, 0);
console.warn = baseWarn;
t.end();
})

test('warning is not printed when attaching propTypes to wrappedComponent', t => {
var msg = [];
var baseWarn = console.warn;
console.warn = (m) => msg = m;

var C = inject(["foo"])(React.createClass({
displayName: 'C',
render: function() {
return e("div", {}, "context:" + this.props.foo);
}
}));

C.wrappedComponent.propTypes = {};

t.equal(msg.length, 0);
console.warn = baseWarn;
t.end();
Expand Down

15 comments on commit 666577b

@mweststrate
Copy link
Member Author

Choose a reason for hiding this comment

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

@jtraub @andykog @Strate would you mind reviewing this proposed solution to #120, #142? See the changelog above. The main idea is: forward assignments to propTypes and defaultProps from a MobXInjector to the original wrapped components, so that the following is valid:

const MyComponent = inject(["someStore"], ({ someStore}) => /*render */)

// pre mobx4:
MyComponent.wrappedComponent.propTypes = { someStore: PropTypes.object.isRequired }

// in mobx4 this is also valid:
MyComponent.propTypes = { someStore: PropTypes.object.isRequired }

This makes decorating a component with inject more transparent, but reading propTypes would always return nothing, which might be weird? Maybe we can detect it is being read as part of Reacts propType validation and then return nothing, otherwise the propTypes of the wrapped component instead?

@andykog
Copy link
Member

@andykog andykog commented on 666577b Nov 9, 2016

Choose a reason for hiding this comment

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

@mweststrate, I wouldn't implement such proxying at all.
It's really weird that propTypes becomes null. Sometimes people do things like:

const MyMenu = ({ items } => <div>{items.map(() => ...)}</div>)
MyList.propTypes = { items: PropTypes.arrayOf(PropTypes.shape({ a, b, c }))

const MySidebar = ({ items } => MyMenu({ items }))
MySidebar.propTypes = { items: MyList.propTypes.items }

But I think, the only way to detect that we are inside rendering is to use react's private stuff, right?

@mweststrate
Copy link
Member Author

@mweststrate mweststrate commented on 666577b Nov 9, 2016 via email

Choose a reason for hiding this comment

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

@mweststrate
Copy link
Member Author

Choose a reason for hiding this comment

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

No that I think of it, not sure why we warn for these static assignments in the first place, actually they are correct? In principle it doesn't matter whether the propTypes are validated on the Injector component or wrapped component. The primary concern of validating propTypes should be to validate external calls of your component, not to validate whether the proper stores are inject / the stores mapper produces the correct result. The same holds for defaultProps I guess.

@jtraub what was the original reason for implementing #88 ?

@mweststrate
Copy link
Member Author

Choose a reason for hiding this comment

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

@jtraub @andykog see: 7f4d5fd

@andykog
Copy link
Member

Choose a reason for hiding this comment

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

@mweststrate, well, 99.9% of time defining propTypes on HOC is not what you want. Can you think of the case where it's needed?

@andykog
Copy link
Member

@andykog andykog commented on 666577b Nov 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@mweststrate, ok, maybe someone can really want to declare propTypes that are not injected with inject as a HOC.propTypes just to seperate them and make more obvious, what needs to be passed from parent component (not an options for me as I use eslint proptypes validation)

@mweststrate
Copy link
Member Author

Choose a reason for hiding this comment

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

I think the tricky part is that people might often not recognize it as being HoC, or don't give much thought about a HoC is happening:

  • const A = observer(props => rendering) -> no hoc, A.propTypes = .. is ok
  • const A = observer(["store"], props => rendering) -> hoc, A.propTypes = .. is not ok ?! But proptypes should be the same, except for "store"
  • const A = inject("store", observer(props => rendering)) -> hoc, A.propTypes = .. is not ok ?! But proptypes should be the same, except for "store"

If I remember correct (should test that, actually the following is different:

// 1
@inject @observer ComponentClass {
   static propTypes = { stuff }
   render() {}
}

// 2
var ComponentClass = inject(observer(React.createClass({
   propTypes: { stuff }
   render: () => {}
}

// 3
@inject @observer ComponentClass {
   render() {}
}
ComponentClass.propTypes = { stuff }

// 4
ComponentClass = inject("stores", observer(() => render))
ComponentClass.propTypes = { stuff }

If I remember correctly (should investigate), 1 and 2 will define propTypes on the inner component, whereas 3 and 4 will define it on the HoC. Which style of component declaration people use might depend much on the ES2015 language features they use / like

@mweststrate
Copy link
Member Author

@mweststrate mweststrate commented on 666577b Nov 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Note that in the following scenario the difference is only notable for the store property, not the 'normal' bold properties, which need to be passed to HoC anyway, just as to the original component:

const UserName = inject("userStore", ({ userStore, bold }) => someRendering())

UserName.propTypes = {
    bold: PropTypes.boolean.isRequired,
    userStore: PropTypes.object.isRequired // will always fail
}

UserName.wrappedComponent.propTypes = {
    bold: PropTypes.boolean.isRequired,
    userStore: PropTypes.object.isRequired // OK
}

@andykog
Copy link
Member

@andykog andykog commented on 666577b Nov 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@mweststrate, that inconsistency is really nasty. But I guess, trying to hide the fact that the HOC is used is a bad choice.

If I remember correctly (should investigate), 1 and 2 will define propTypes on the inner component

It will define propTypes on inner component if you place transform-decorators-legacy after transform-class-properties, in your config. Not sure about non-legacy decorators spec

@mweststrate
Copy link
Member Author

Choose a reason for hiding this comment

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

But I guess, trying to hide the fact that the HOC is used is a bad choice.

Well it is not a deliberate choice, it is just that most people will consider the HoC (if there is one) in their mind as "the XyzComponent", because from the outside, a HO wrapped component looks just the same as a component. It's called the same etc, but propTypes and defaultProps suddenly need to be configured on a different place. I think it is partially unavoidable confusion related to HoC's.

@andykog
Copy link
Member

Choose a reason for hiding this comment

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

@mweststrate, promoting people to think HoC's is their original component is harmful, HoC is nothing to do with the original component. Here is an example when this abstraction leaks: https://jsfiddle.net/Sl1v3r/qqase12u/

Maybe, the amount on confusion could be lower if we deprecate observer([injectedProps], component), so that observer always only makes component observable, inject always returns HoC?

It's called the same etc

I would also give HoC's different name, something like ${originalName}Injector

@mweststrate
Copy link
Member Author

Choose a reason for hiding this comment

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

@andykog Ok, I think you convinced me :). I think observer(["store"]) should probably indeed be deprecated in v4, it only adds to the confusion.

Nonetheless, giving a warning when assigning PropTypes to the HoC might still be too aggressive? Basically it is not bad to assign propTypes to the HoC? The name is already improved btw :)

For the name, this will be part of v4 as well:

inject

@andykog
Copy link
Member

Choose a reason for hiding this comment

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

giving a warning when assigning PropTypes to the HoC might still be too aggressive?

I really don't know, as I said, I believe most of the time assigning propTypes on HoC is not what user intended, but I can imagine such usecase (seperating general propTypes from injected).

I thought about alternative, but its a bit to tricky: we could intercept propTypes, declared on HoC and wrap all the validators in additional function like this:

(...args) => {
  const result = originalValidator(...args);
  if (result instanceof Error)
    return new Error(
      result.message
      + "Have you declared propType on HoC, dummy?"
    );
  return result;
}

@jtraub
Copy link
Contributor

@jtraub jtraub commented on 666577b Nov 11, 2016

Choose a reason for hiding this comment

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

@jtraub what was the original reason for implementing #88 ?

@mweststrate

Initially, I wanted to have transparent passing of propTypes and defaultProps (see #70 (comment))

But then you've implemented wrappedComponent property so I needed a way of preventing me from MyComponent.propTypes = { ..

Please sign in to comment.