Components can't safely import React

[ericelliott]

I just noticed a pretty serious flaw in react-stampit:

Component source files should never import React, because that could lead to multiple instances and multiple versions of React in the same project -- a situation that causes bugs. Instead, it should be passed into the stamp when an instance is instantiated (dependency injection). You'll need to work some magic to expose the React.PropTypes for the react-stamp to use.

A fix for this issue will be a breaking change, and any users who've managed to find this already should upgrade immediately to the fixed version to prevent bugs in their applications.

Example from react-hello:

import React from 'react'; // we can't do this...
import stampit from 'react-stampit';

const {
  string,
  func
} = React.PropTypes;

[troutowicz]

Is this not what components do with React.createClass? I wasn't aware of bugs resulting from this.

[ericelliott]

No, not if the mixins are well designed. Mixins should never require their own copy of React, for just this reason. One of the main reasons that composing real components is a better idea than using Mixins.

Anything that would cause two React instances should be an anti-pattern just because the browser may have to load React twice, so even if it didn't actually cause bugs in software which is one of the most common issues new users face, this should still be considered a bug.

[troutowicz]

Thanks for the link. I'll work on the change. :+1:

[troutowicz]

So this is one way we can fix it.

• React.PropTypes

  Export a React object stub from react-stampit that contains an empty object property PropTypes. The developer would import this in components that require it. Then the real React lib would take priority when passed as an argument to the factory.

• API

test('stampit(props)(React)', (t) => {
  t.plan(1);

  const stampFactory = stampit({
    propTypes: {
      foo: React.PropTypes.string
    },

    render() {
      return null;
    }
  });
  const stamp = stampFactory(React);

  t.ok(
    stampit.isStamp(stamp),
    'should return a stamp'
  );
});

We could maybe create a convenience method on the stamp factory for some simplicity.

const stamp = stampit({
  propTypes: {
    foo: React.PropTypes.string
  },

  render() {
    return null;
  }
}).inject(React);

[ericelliott]

I don't understand those examples. Both depend on React directly in the definition of the stamp, and then pass React into the stamp during instantiation.

The point of this issue is that React won't be available to depend on when the user is defining the stamp.

[troutowicz]

My example is partially wrong. The .inject method is not needed and actually doesn't make sense. Let me split things up.

Edit: This actually won't work either, need to rethink how to handle React.PropTypes...

component.jsx

import { React } from 'react-stampit';

export default stampit({
  propTypes: {
    foo: React.PropTypes.string
  },

  render() {
    return null;
  }
});

app-client.jsx

import React from 'react';
import componentFactory from './component';

const Component = componentFactory(React);

React.render(
    <Component />,
    document.getElementById('content')
);

[ericelliott]

That makes more sense, but wouldn't this be more clear?

import { PropTypes } from 'react-stampit';

export default stampit({
  propTypes: {
    foo: PropTypes.string
  },

  render() {
    return null;
  }
});

Also, PropTypes could not be "empty" - it would actually have to contain the full React.PropTypes object structure so that things like PropTypes.func.required don't throw exceptions.

[ericelliott]

This wouldn't be such a headache if React itself was a bit more modular. PropTypes should really be its own module so we could just do something like import PropTypes from 'react-proptypes'; and we wouldn't have to worry about multiple versions of the behavioral bits of React floating around in our apps...

[troutowicz]

It does, but I was trying to keep things familiar. Either way, it doesn't matter because that approach will fail. The properties will be undefined.

[troutowicz]

PropTypes should really be its own module

This may be what we need to create.

[ericelliott]

Who says the properties have to be undefined? As I mentioned in my comment above, you'd have to supply the full React.PropTypes object structure for this to work. =)

[troutowicz]

Ah, missed you saying that, our posts came in a bit out of order. ;)

[ericelliott]

:+1:

[troutowicz]

@ericelliott I feel I have to say one last thing here before continuing this breaking change. The only time multiple instances of React would occur is if an app, which has React as a dependency, has dependencies that also have their own React dependency, right? So is the concern here that components/mixins would not be safely distributable if React was a dependency? That's the only reason I can think of.

[ericelliott]

Not the only reason, because many apps (including mine) source React from CDN (likely already cached by browsers).

I believe both Browserify and Webpack automatically dedupe dependencies, so this should theoretically only be a dependency problem when a component or mixin imports a conflicting version of React, which is unfortunately a common scenario that has led to many GitHub issues and StackOverflow questions.

[troutowicz]

Looks like someone has made a module for PropTypes.

[troutowicz]

@ericelliott Created a refactor branch. I need to work through all the tests, and update docs. I did update the example in the README though.

[ericelliott]

Nice!

[troutowicz]

@ericelliott I just realized PR #14 is largely not needed. Components can be made factories by the developer, it does not and should not be the libraries responsibility IMO. If the library handles this, then components that have a render function returning something other than null will error because React is not defined inside the component. (React.createElement)

Creating a factory component without the PR

import { PropTypes } from 'prop-types';

export default React => stampit(React, {
  propTypes: {
    foo: PropTypes.string
  },

  render() {
    return null;
  }
});

Creating a factory component with the PR

import { PropTypes } from 'prop-types';

export default React => stampit({
  propTypes: {
    foo: PropTypes.string
  },

  render() {
    return null;
  }
})(React);

The latter results in a extra function call and a breaking change in the API.

[ericelliott]

This looks like the cleanest solution to the problem.

import { PropTypes } from 'prop-types';

export default React => stampit(React, {
  propTypes: {
    foo: PropTypes.string
  },

  render() {
    return null;
  }
});

Want to update the docs to clarify this?

[troutowicz]

Yeah, I'm going to take any useful changes from this PR and update the docs.

[ericelliott]

:sunglasses:

[troutowicz]

Closed, see #15