carteb / carte-blanche

An isolated development space with integrated fuzz testing for your components. See them individually, explore them in different states and quickly and confidently develop them.
https://www.youtube.com/watch?v=6g3-TQ6aaw8
MIT License
1.5k stars 47 forks source link

Webpack tree shaking support #288

Open FezVrasta opened 8 years ago

FezVrasta commented 8 years ago

(I've temporary disabled CommonChunks to try Carte Blanche)

When I've added a variation to my Button component, I get:

warning.js:44Warning: Failed propType: Invalid prop `component` of type `object` supplied to `Playground`, expected `function`.
common.js.js:1 Uncaught SyntaxError: Unexpected token <
warning.js:44 Warning: React.createElement: type should not be null, undefined, boolean, or number. It should be a string (for DOM elements) or a ReactClass (for composite components).
invariant.js:38 Uncaught Invariant Violation: Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: object.
ReactCompositeComponent.js:772 Uncaught TypeError: Cannot read property '_currentElement' of null

My Carte Blanche configuration is:

new CarteBlanche({
      componentRoot: './app/components',
      plugins: [
        new ReactPlugin({ injectTags: ['<script src="common.js.js"></script>'] }),
      ],
    }),

My Button component is:

/**
 *
 * Button
 *
 */

import React, { PropTypes } from 'react';
import styleguide from './styleguide';

// Component styling
import CSSModules from 'react-css-modules';
import styles from './styles.styl';

// Component class
@CSSModules(styles, { allowMultiple: true })
class Button extends React.Component {
  static styleguide = styleguide;

  // Default component properties
  static defaultProps = {
    type: 'button',
    primary: false,
  };

  render() {
    // Style of Button's inner element
    let innerStyleName = 'Button__inner';
    if (this.props.primary) innerStyleName += ` ${innerStyleName}--primary`;

    // Type of the button
    const type = this.props.type;

    return (
      <button
        type={type}
        styleName='Button'
        className={this.props.className}
        onClick={this.props.onClick}
      >
        <div styleName={innerStyleName}>
          {this.props.children}
        </div>
      </button>
    );
  }
}

// propTypes definition
Button.propTypes = {
  // The HTML tag button type
  type: PropTypes.oneOf(['button', 'submit', 'reset']),

  // Defines if Button will have the `primary` style
  primary: PropTypes.bool,

  // Function executed on click
  onClick: PropTypes.func,

  // Optional className applied to Button
  className: PropTypes.string,

  // This is the content of Button (usually text)
  children: PropTypes.node.isRequired,
};

export default Button;

I'm using as base react-boilerplate.

mxstbr commented 8 years ago

Try removing the link to the common.js.js files, I think that's what's throwing the error. (as seen by the line saying common.js.js:1 Uncaught SyntaxError: Unexpected token <)

FezVrasta commented 8 years ago

Removing that line I get:

image

dleen commented 8 years ago

I'm getting this too. I'm not using commonjs. I wish I could be more helpful but I don't have any idea where to start looking with this one :(

Warning: Failed propType: Invalid prop 'component' of type 'object' supplied to 'Playground', expected 'function'.

Warning: React.createElement: type should not be null, undefined, boolean, or number. It should be a string (for DOM elements) or a ReactClass (for composite components).
dleen commented 8 years ago

This might have something to do with either webpack 2.0 or react-hot-loader 3.0

ChristopherBiscardi commented 8 years ago

I'm also seeing this with webpack 2, (no hot-loader 3)

mxstbr commented 8 years ago

That's a really annoying bug. :confused: Could one of you possibly provide a stripped down test project so we can replicate this?

ChristopherBiscardi commented 8 years ago

I'll make a note to produce a minimal case. Won't get to it until tomorrow though.

mxstbr commented 8 years ago

Awesome, thanks!

ChristopherBiscardi commented 8 years ago

I've been digging a bit and currently tracing back from here where

resources.compiledComponent(componentFile)

is an empty object {}, which is why componentData.getCompiledComponent is returning an empty object as well.

Right now I think this points to the source of the issue.

mxstbr commented 8 years ago

@ChristopherBiscardi oh wow, thanks for digging in!

If I understand that correctly, an if (componentFile) {} should fix that? /cc @nikgraf

ChristopherBiscardi commented 8 years ago

I'm not so sure, componentFile is well defined (in this case app/mark/Test.js), and resources.compiledComponent seems to be working as intended. The value in the contextual require is an empty object. IMO it seems similar to cyclic dependency issues I've seen in the past, where the loaded file is loaded as {} instead of the content of the module.

Everything seems to be pointing at the right location except this is included at the bottom of the Test.js module:

    /* unused harmony default export */ var _unused_webpack_default_export = Test;
/***/ },
/* 349 */
/***/ function(module, exports, __webpack_require__) {

    "use strict";
    /* harmony import */ var __WEBPACK_IMPORTED_MODULE_0_react__ = __webpack_require__(8);
    /* harmony import */ var __WEBPACK_IMPORTED_MODULE_0_react___default = __WEBPACK_IMPORTED_MODULE_0_react__ && __WEBPACK_IMPORTED_MODULE_0_react__.__esModule ? function() { return __WEBPACK_IMPORTED_MODULE_0_react__['default'] } : function() { return __WEBPACK_IMPORTED_MODULE_0_react__; };
    /* harmony import */ Object.defineProperty(__WEBPACK_IMPORTED_MODULE_0_react___default, 'a', { get: __WEBPACK_IMPORTED_MODULE_0_react___default });
    var _createClass = function () { function defineProperties(target, props) { for (var i = 0; i < props.length; i++) { var descriptor = props[i]; descriptor.enumerable = descriptor.enumerable || false; descriptor.configurable = true; if ("value" in descriptor) descriptor.writable = true; Object.defineProperty(target, descriptor.key, descriptor); } } return function (Constructor, protoProps, staticProps) { if (protoProps) defineProperties(Constructor.prototype, protoProps); if (staticProps) defineProperties(Constructor, staticProps); return Constructor; }; }();

    function _classCallCheck(instance, Constructor) { if (!(instance instanceof Constructor)) { throw new TypeError("Cannot call a class as a function"); } }

    function _possibleConstructorReturn(self, call) { if (!self) { throw new ReferenceError("this hasn't been initialised - super() hasn't been called"); } return call && (typeof call === "object" || typeof call === "function") ? call : self; }

    function _inherits(subClass, superClass) { if (typeof superClass !== "function" && superClass !== null) { throw new TypeError("Super expression must either be null or a function, not " + typeof superClass); } subClass.prototype = Object.create(superClass && superClass.prototype, { constructor: { value: subClass, enumerable: false, writable: true, configurable: true } }); if (superClass) Object.setPrototypeOf ? Object.setPrototypeOf(subClass, superClass) : subClass.__proto__ = superClass; }

    var Test = function (_Component) {
      _inherits(Test, _Component);

      function Test() {
        _classCallCheck(this, Test);

        return _possibleConstructorReturn(this, Object.getPrototypeOf(Test).apply(this, arguments));
      }

      _createClass(Test, [{
        key: 'render',
        value: function render() {
          return __WEBPACK_IMPORTED_MODULE_0_react___default.a.createElement(
            'div',
            null,
            this.props.title
          );
        }
      }]);

      return Test;
    }(__WEBPACK_IMPORTED_MODULE_0_react__["Component"]);

    Test.propTypes = {
      title: __WEBPACK_IMPORTED_MODULE_0_react___default.a.PropTypes.string.isRequired
    };
    /* unused harmony default export */ var _unused_webpack_default_export = Test;
ChristopherBiscardi commented 8 years ago

IE: it seems that webpack doesn't think Test.js is being used and attempts to shake it off the tree as a result.

ChristopherBiscardi commented 8 years ago

Temporary fix is to use module.exports instead of export default

-- Bug

import React, { Component } from 'react';

export default class Test extends Component {
  static propTypes = {
    title: React.PropTypes.string.isRequired
  }
  render() {
    return (
      <div>{this.props.title}</div>
    )
  }
}

-- Good

import React, { Component } from 'react';

module.exports =  class Test extends Component {
  static propTypes = {
    title: React.PropTypes.string.isRequired
  }
  render() {
    return (
      <div>{this.props.title}</div>
    )
  }
}
ChristopherBiscardi commented 8 years ago

Alternatively, one can remove es2015-webpack in favor of the full es2015 babel preset (for example, if you're using react-boilerplate 3. This uses babel to transpile the imports and you lose tree-shaking. One idea is to use es2015 when running carte and es2015-webpack when compiling for prod using .babelrc's support for envs.

dleen commented 8 years ago

Can confirm that changing es2015-webpack to es2015 does indeed fix the problem. The tree shaking makes sense. :metal:

mxstbr commented 8 years ago

That's annoying though, we shouldn't be forcing people to change their webpack config for carte blanche.

Thanks so much for digging in @ChristopherBiscardi! We'll have to think about how to fix this properly then, any ideas?

ChristopherBiscardi commented 8 years ago

It seems that webpack is handling require.context correctly in a minimal test repo. Nothing gets shaken from the context require while other files get properly shaken.

The place I'm looking right now is the dynamic-resolve loader (dynamic-resolve should probably be renamed to -loader, right?), which is responsible for replacing the placeholder file with the code that returns the empty object.

One technique that may work is scooping up the files and injecting them as an array instead of injecting the require.context. I don't actually know if that would work, but it's one way I've handled situations like this without using .context in the past.

const components = [
  require('Test.js'),
  require('OtherComponent.js')
]

Is user-bundle being processed twice?

FezVrasta commented 8 years ago

I'm using es2015 I guess:

  babel: {
    presets: ['es2015', 'react', 'stage-0'],
  },

But I still have the problem.

mxstbr commented 8 years ago

Alright, with CommonsChunkPlugin and express.js support done (released as 0.2), this is (I think) the last thing to fix for it to be compatible with react-boilerplate. Again, thanks for the investigation @ChristopherBiscardi, that's really really helpful!

Ideally, we wouldn't require people to adapt their code or configs to Carte Blanche. It should just work™.

I understand this is quite a hard requirement, and I also think swapping out es2015-webpack for es2015 in development is a fair drawback but I'd rather not people have to do it. Any ideas how we could avoid this issue without people adapting their config?

FezVrasta commented 8 years ago

@mxstbr https://github.com/carteb/carte-blanche/issues/288#issuecomment-224372030 is this a viable solution in your opinion?