1.1.4 is broken

[kyeotic]

Your patch release changed the module exports. react-document-title is now broken

×
TypeError: withSideEffect is not a function
./node_modules/react-document-title/index.js
node_modules/react-document-title/index.js:37
  34 |   }
  35 | };
  36 | 
> 37 | module.exports = withSideEffect(
  38 |   reducePropsToState,
  39 |   handleStateChangeOnClient
  40 | )(DocumentTitle);

[kyeotic]

@lourd I think this was your recent change to esm: edbfaf4#diff-1fdf421c05c1140f6d71444ea2b27638

[lourd]

Thanks for your report Tim.

The source now uses esm, but the build output as specified by the "main" key in package.json did not change. It still uses CJS.

We don't include lib/ in source control, but you can see the contents here https://unpkg.com/[email protected]/lib/index.js. You can compare that to 1.1.3 here https://unpkg.com/[email protected]/lib/index.js.

I just cloned react-document-title and ran its tests against v1.1.4 and didn't have an issue. I've also double-checked that I can clone this repo, build it, and require it without an issue.

git clone https://github.com/gaearon/react-side-effect
cd react-side-effect
yarn install
node
let withSideEffect = require('./')
withSideEffect.toString()

This prints out the function contents as I would expect.

[kyeotic]

Ok. Well if I force react-side-effect to install 1.1.3 I can run. If it uses 1.1.4 I get this error. I have changed no other code.

[lourd]

Can you paste the contents of react-side-effect/lib/index.js in your node_modules/?

[kyeotic]

Ok, something is caching something. Maybe webpack. If I install with 1.1.4 then paste in either the unpkg 1.1.3 or 1.1.4 I still get that error.

If I npm [email protected] the issue goes away...

[kyeotic]

Ok, I tested it on my build server just to be sure. If 1.1.3 is installed everything is fine. If I let 1.1.4 install transitively it breaks the tests. I don't have a good explanation for why, but it is broken. Judging by the reactions to the top post I am not alone.

Would you consider unpublishing until it gets sorted out?

[kyeotic]

Maybe publish it as a beta so testing can continue from npm installation?

[lourd]

I would really rather figure out the root cause than start swapping versions around. I've tested 1.1.4 against react-document-title, react-helmet, and react-document-meta and all of them passed their test suites without a problem.

I'm sorry there are some builds breaking. Folks who are having issues can pin their version to 1.1.3 for now.

Other questions on what you're seeing:

• Is there a chance your build is now consuming the ESM format at lib/index.es.js? That's now specified in the "module" field of 1.1.4's package.json. That still shouldn't be a problem, but maybe it is.
• Do you have multiple dependencies depending on react-side-effect? Is this being caused by multiple versions of the library?

[kyeotic]

I do not have multiple dependencies, but I am using the create-react-app webpack configuration, so maybe it is using the esm version.

Im going to have to version pin about 6 projects. And that's just me, 3 other people have upvoted this. I think you should care more about the broken builds you have caused.

[kyeotic]

You broke transitive dependants in a semver patch. Builds will start failing with zero code changes. You are literally accepting unit tests results over real world failures.

[lourd]

I understand your frustration and I'm eager to figure out why it's happening. If projects have their dependencies locked down then they won't start failing. Can you, or any of the other folks who are having this issue, share a reproduction?

[kyeotic]

Yes, if instead of allowing semver to update security patches they lock down even their transitive dependencies (which react-document-title does not do) they will not be affected. I don't think you should be using that as a shield though.

[kyeotic]

I will have to trim out our app code to make a repro for you, but right now I have a bunch of projects to update. I will have more time to help you debug tomorrow.

[mikecousins]

Yup, we're getting this too. Our builds today are all failing due to this and pinning react-side-effect to 1.1.3 fixes it.

[me717]

Likewise. This is causing our projects with react-document-title to fail.

[lourd]

Thanks for your reports @mikecousins and @me717. Are your build failures also coming through react-document-title or another module?

[mikecousins]

We see it through react-document-title and also survey-react.

[me717]

We are seeing it through react-document-title

[lourd]

Mike, can you paste what you're seeing from survey-react? I'm not seeing react-side-effect or react-document-title or another related module else as a dependency of that project.

[mikecousins]

On second thought it might just be messed. Here's what I'm seeing:

[lourd]

Hm. Can't tell much from that 😕

Can you paste the contents of your node_modules/react-side-effect/lib/index.js file?

[mikecousins]

With it pinned at 1.1.3 or floating?

[lourd]

Floating

[mikecousins]

'use strict';

var _react = require('react');

var _react2 = _interopRequireDefault(_react);

var _exenv = require('exenv');

var _exenv2 = _interopRequireDefault(_exenv);

var _shallowequal = require('shallowequal');

var _shallowequal2 = _interopRequireDefault(_shallowequal);

function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }

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; }

module.exports = function withSideEffect(reducePropsToState, handleStateChangeOnClient, mapStateOnServer) {
  if (typeof reducePropsToState !== 'function') {
    throw new Error('Expected reducePropsToState to be a function.');
  }
  if (typeof handleStateChangeOnClient !== 'function') {
    throw new Error('Expected handleStateChangeOnClient to be a function.');
  }
  if (typeof mapStateOnServer !== 'undefined' && typeof mapStateOnServer !== 'function') {
    throw new Error('Expected mapStateOnServer to either be undefined or a function.');
  }

  function getDisplayName(WrappedComponent) {
    return WrappedComponent.displayName || WrappedComponent.name || 'Component';
  }

  return function wrap(WrappedComponent) {
    if (typeof WrappedComponent !== 'function') {
      throw new Error('Expected WrappedComponent to be a React component.');
    }

    var mountedInstances = [];
    var state = void 0;

    function emitChange() {
      state = reducePropsToState(mountedInstances.map(function (instance) {
        return instance.props;
      }));

      if (SideEffect.canUseDOM) {
        handleStateChangeOnClient(state);
      } else if (mapStateOnServer) {
        state = mapStateOnServer(state);
      }
    }

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

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

        return _possibleConstructorReturn(this, _Component.apply(this, arguments));
      }

      // Try to use displayName of wrapped component
      SideEffect.peek = function peek() {
        return state;
      };

      // Expose canUseDOM so tests can monkeypatch it


      SideEffect.rewind = function rewind() {
        if (SideEffect.canUseDOM) {
          throw new Error('You may only call rewind() on the server. Call peek() to read the current state.');
        }

        var recordedState = state;
        state = undefined;
        mountedInstances = [];
        return recordedState;
      };

      SideEffect.prototype.shouldComponentUpdate = function shouldComponentUpdate(nextProps) {
        return !(0, _shallowequal2.default)(nextProps, this.props);
      };

      SideEffect.prototype.componentWillMount = function componentWillMount() {
        mountedInstances.push(this);
        emitChange();
      };

      SideEffect.prototype.componentDidUpdate = function componentDidUpdate() {
        emitChange();
      };

      SideEffect.prototype.componentWillUnmount = function componentWillUnmount() {
        var index = mountedInstances.indexOf(this);
        mountedInstances.splice(index, 1);
        emitChange();
      };

      SideEffect.prototype.render = function render() {
        return _react2.default.createElement(WrappedComponent, this.props);
      };

      return SideEffect;
    }(_react.Component);

    SideEffect.displayName = 'SideEffect(' + getDisplayName(WrappedComponent) + ')';
    SideEffect.canUseDOM = _exenv2.default.canUseDOM;


    return SideEffect;
  };
};

[lourd]

That's v1.1.3. Can you verify that your build is using v1.1.4 and paste that?

[mikecousins]

Oops, sorry, didn't update:

'use strict';

function _interopDefault (ex) { return (ex && (typeof ex === 'object') && 'default' in ex) ? ex['default'] : ex; }

var React = require('react');
var React__default = _interopDefault(React);
var ExecutionEnvironment = _interopDefault(require('exenv'));
var shallowEqual = _interopDefault(require('shallowequal'));

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; }

function withSideEffect(reducePropsToState, handleStateChangeOnClient, mapStateOnServer) {
  if (typeof reducePropsToState !== 'function') {
    throw new Error('Expected reducePropsToState to be a function.');
  }
  if (typeof handleStateChangeOnClient !== 'function') {
    throw new Error('Expected handleStateChangeOnClient to be a function.');
  }
  if (typeof mapStateOnServer !== 'undefined' && typeof mapStateOnServer !== 'function') {
    throw new Error('Expected mapStateOnServer to either be undefined or a function.');
  }

  function getDisplayName(WrappedComponent) {
    return WrappedComponent.displayName || WrappedComponent.name || 'Component';
  }

  return function wrap(WrappedComponent) {
    if (typeof WrappedComponent !== 'function') {
      throw new Error('Expected WrappedComponent to be a React component.');
    }

    var mountedInstances = [];
    var state = void 0;

    function emitChange() {
      state = reducePropsToState(mountedInstances.map(function (instance) {
        return instance.props;
      }));

      if (SideEffect.canUseDOM) {
        handleStateChangeOnClient(state);
      } else if (mapStateOnServer) {
        state = mapStateOnServer(state);
      }
    }

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

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

        return _possibleConstructorReturn(this, _Component.apply(this, arguments));
      }

      // Try to use displayName of wrapped component
      SideEffect.peek = function peek() {
        return state;
      };

      // Expose canUseDOM so tests can monkeypatch it


      SideEffect.rewind = function rewind() {
        if (SideEffect.canUseDOM) {
          throw new Error('You may only call rewind() on the server. Call peek() to read the current state.');
        }

        var recordedState = state;
        state = undefined;
        mountedInstances = [];
        return recordedState;
      };

      SideEffect.prototype.shouldComponentUpdate = function shouldComponentUpdate(nextProps) {
        return !shallowEqual(nextProps, this.props);
      };

      SideEffect.prototype.componentWillMount = function componentWillMount() {
        mountedInstances.push(this);
        emitChange();
      };

      SideEffect.prototype.componentDidUpdate = function componentDidUpdate() {
        emitChange();
      };

      SideEffect.prototype.componentWillUnmount = function componentWillUnmount() {
        var index = mountedInstances.indexOf(this);
        mountedInstances.splice(index, 1);
        emitChange();
      };

      SideEffect.prototype.render = function render() {
        return React__default.createElement(WrappedComponent, this.props);
      };

      return SideEffect;
    }(React.Component);

    SideEffect.displayName = 'SideEffect(' + getDisplayName(WrappedComponent) + ')';
    SideEffect.canUseDOM = ExecutionEnvironment.canUseDOM;


    return SideEffect;
  };
}

module.exports = withSideEffect;

[lourd]

Well, it's definitely got the export:

module.exports = withSideEffect;

You've tried deleting your node_modules/ and re-installing and still get the same error?

[mikecousins]

Yup, still the same error. Our CD server builds from scratch and our dev instance deployments are messed now.

[lourd]

Can you include the other details in your build?

• OS version
• npm or yarn version
• Node version

Of course a reproduction repo would be the most helpful

[mikecousins]

• Windows 10 Pro 1709 16299.192
• npm 5.6.0
• yarn 1.3.2
• node 9.5.0

It's also failing for co-workers on OS X and our Linux build server (CentOS). Build server is probably running an older version of node/npm.

[ek5000]

I think I've narrowed it down. react-document-title doesn't use a transpiler, so its require('react-side-effect') is used "bare", without the _interopRequireDefault that most projects seem to use. But as of 1.1.4, this project now exposes "module" in it's package.json, which uses a default es2015 export.

So if you're using a "module" aware build system, such as webpack>=2, then require('react-side-effect') will return an object with default as a key.

[lourd]

Hey @ek5000, thanks for helping out.

I think you're right that it's related to webpack and the presence of the "module" field, but I don't quite understand why, yet. lib/index.js, the file specified as "main", simply uses module.exports = fn. Requiring the module from a Node REPL, described earlier, works fine. You get the function, not an object.

What versions of webpack are people using?

[mikecousins]

We're using 3.11.0.

[ek5000]

We're using 2.7.0. As for why, I think it's because webpack defaults first to using "module", then falls back to using "main". You can change that behavior by setting mainFields, but this sets it for all dependencies, and hurts tree shaking.

[lourd]

Alright, I just tried the test suite for react-document-title and react-helmet with v1.1.4 on a Windows machine and it worked fine.

Then I created a small test repo to test with webpack-bundled code, and it also worked fine. I tested against Webpack v2 and v3. No luck.

Let me be clear — I believe all of you, I'm just trying to reproduce the issue.

[ek5000]

Try it with this change

-import withSideEffect from 'react-side-effect'
+const withSideEffect = require('react-side-effect')

output:

> @ test /home/ek/opensource/react-side-effect-bug-hunt
> webpack && node bundle.js

Hash: 4cb788756f8a7c1cab90
Version: webpack 3.11.0
Time: 141ms
    Asset     Size  Chunks             Chunk Names
bundle.js  77.4 kB       0  [emitted]  main
   [6] ./index.js 127 bytes {0} [built]
    + 14 hidden modules
withSideEffect is...
[object Object]

The latter is what react-document-title has.

[lourd]

Bingo. I'm gonna hit up one of the webpack guys to figure this out.

[mayank23]

I think you need to build a commonjs compatible version and point the "main" property in the package.json to it. Example: https://github.com/arnaudbenard/redux-mock-store/blob/master/package.json#L5

[lourd]

Hey Mayank, thanks for the suggestion.

The "main" property does point to a CJS version, lib/index.js. It appears that webpack is using the ESM version when using require instead of the CJS version.

[TheLarkInn]

Hey hey @lourd.

See here: https://webpack.js.org/configuration/resolve/#resolve-mainfields

Also why is this breaking peoples builds, is this because folks are still doing CJS require on this module?

[lourd]

I've gone ahead and published 1.1.5 which doesn't have a "module" property in the package.json, which resolves the issue and will stop builds from breaking while we figure this out.

Because react-document-title is using CJS, builds were breaking.

How should I resolve this? Even if I update react-document-title there may be other modules doing the same thing. Publish a new major version?

[TheLarkInn]

I did submit this anyways, but likely given the context should be a SemVer Major since it is breaking change.

gaearon/react-document-title#55

[realityking]

Urgh, I wish I had seen #46. This is the exact issue I've hit with some other packages that I alluded to in #42 (comment). Sorry, I should have explained that in more detail.