Skip to content
This repository has been archived by the owner on Jun 15, 2023. It is now read-only.

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Question: does brunch support conditional require ? (use case in react@15.4 above) #1591

Closed
c0b opened this issue Dec 18, 2016 · 17 comments
Closed
Labels

Comments

@c0b
Copy link

c0b commented Dec 18, 2016

Description

What's the essence of the issue?

➸ brunch --version
2.9.1

Expected behavior

Tell us what you think should happend.

support latest react:

  "dependencies": {
    "material-ui": "^0.16.5",
    "react": "^15.4.1",
    "react-dom": "^15.4.1",
    "react-redux": "^5.0.1",
    "react-tap-event-plugin": "^2.0.1",
    "redux": "^3.6.0"
  },
  "devDependencies": {
    "auto-reload-brunch": "^2.0.0",
    "babel-brunch": "~6.0.0",
    "babel-preset-es2015": "~6.9.0",
    "babel-preset-react": "~6.11.1",
    "brunch": "^2.4.0",
    "clean-css-brunch": "^2.0.0",
    "css-brunch": "^2.0.0",
    "javascript-brunch": "^2.0.0",
    "uglify-js-brunch": "^2.0.0"
  }

Actual behavior

Tell us what actually happens.

I met a same issue as this one: error in browser console: but his solution of remove node_modules/ and re-compile doesn't resolve problem here; it is always reproduce

Uncaught Error: Cannot find module 'react-dom/lib/ReactPerf' from 'react/lib/ReactAddonsDOMDependencies.js'

http://stackoverflow.com/questions/40953011/browser-console-error-cannot-find-module-react-dom-lib-reactperf-from-react

If at all possible, please create a small demo app on GitHub that demonstrates the issue so it's easier for us to check and debug.

brunch new -s react; npm install --save react@latest react-dom@latest material-ui ...

the default empty template compiles and runs fine; but once I added the first button, the ui reload failed with above error in console

Environment

  1. Brunch: 2.9.1
  2. Node: 6.9.2
  3. NPM: (npm v2 is not supported)
  4. Operating system: Linux

package.json contents

{
  "name": "your-app",
  "private": true,
  "version": "0.0.1",
  "scripts": {
    "start": "brunch watch --server -P 3334",
    "prod": "brunch build --production"
  },
  "dependencies": {
    "material-ui": "^0.16.5",
    "react": "^15.4.1",
    "react-dom": "^15.4.1",
    "react-redux": "^5.0.1",
    "react-tap-event-plugin": "^2.0.1",
    "redux": "^3.6.0"
  },
  "devDependencies": {
    "auto-reload-brunch": "^2.0.0",
    "babel-brunch": "~6.0.0",
    "babel-preset-es2015": "~6.9.0",
    "babel-preset-react": "~6.11.1",
    "brunch": "^2.4.0",
    "clean-css-brunch": "^2.0.0",
    "css-brunch": "^2.0.0",
    "javascript-brunch": "^2.0.0",
    "uglify-js-brunch": "^2.0.0"
  }
}

brunch config contents

the default from template:

module.exports = {
  files: {
    javascripts: {
      joinTo: {
        'vendor.js': /^(?!app)/,
        'app.js': /^app/
      }
    },
    stylesheets: {joinTo: 'app.css'}
  },

  plugins: {
    babel: {presets: ['es2015', 'react']}
  }
};

Other useful files, when present (log, bower.json etc.)

I suspect the problem is in brunch because I checked the transpiled public/vendor.js file, I can see the module 'react/lib/ReactAddonsDOMDependencies.js' is registered but I am not seeing the module 'react-dom/lib/ReactPerf' registered,

we can see the file content here: require('react-dom/lib/ReactPerf') is conditionally loaded when not in production mode, which is needed during development, so question here is does brunch support this kind of conditionally loading a module? if so, what changes in brunch-config is required?

https://unpkg.com/react@15.4.1/lib/ReactAddonsDOMDependencies.js

if (process.env.NODE_ENV !== 'production') {
  var ReactPerf = require('react-dom/lib/ReactPerf');
  var ReactTestUtils = require('react-dom/lib/ReactTestUtils');
@c0b
Copy link
Author

c0b commented Dec 18, 2016

I've just had a quick test with webpack doesn't have this problem, but a default config with webpack seems just packing everything in the bundle? the size is 1.4MB vs brunch can build a dev version of ~930KB;

1.7M Dec 17 23:33 build.js.map
1.4M Dec 17 23:33 build.js
if (true) {
  var ReactPerf = __webpack_require__(623);
  ...

@c0b
Copy link
Author

c0b commented Dec 19, 2016

it seems this project is out of maintenance? from current open issues I can see multiple ones remain open after 10+ days without any response; on community support on StackOverflow (http://brunch.io/support http://stackoverflow.com/questions/tagged/brunch) or http://ost.io/@brunch/brunch it is similar that many questions remain open for a long time;

As a new user with brunch, partially convinced by http://brunch.io/docs/why-brunch#brunch-vs-webpack when brunch works I feel it works better than webpack, and the transplied vendor.js is smaller; but when it doesn't work like in #1592 #1591 #1588 #1587 #1585 #1581 #1580 #1573 the feeling is helpless,

ping @paulmillr @goshakkk @shvaikalesh @marcioj as maintainers from this project's 2016 major contributors https://github.com/brunch/brunch/graphs/contributors?from=2016

I think will have to switch back to webpack if can't resolve this in another few days
to Paul's comment here #1234 (comment)

The reason for small popularity is lack of marketing. Brunch is not getting mentioned by "big folks" even though it's been around since 2011 (Grunt appeared in Apr 2012).

I can't agree because "big folks" here is referred passively if none of those "big folks" noticed this project, why not someone here proactively push this project to front of their eyes; or like this podcast on webpack2 do some self-marketing: many kinds of evangelist can be done earlier
https://changelog.com/podcast/233 (EPISODE #233 / DEC 17 2016 / 80 MIN, Sean Larkin joined the show to talk about Webpack)

Javascript is the most thriving community I've ever seen, on every functionality domain there are tons of projects competing each other, if you don't actively taking care of it, let it being swallowed by tons of bugs, be dying of fading out is sooner or later

@goshacmd
Copy link
Contributor

Brunch is an open source project that is maintained in our spare time; some of the contributions are sponsored out of Paul's own pocket.

You may find it hard to believe but most of us have real lives with partners and friends; as well as work or contracts that we need to pay our bills, so it's no surprise the time we have for side projects and OSS is limited and we may choose to prioritize other projects of ours.

It's an open source project, not a cafe. We don't owe you—or anyone else, for that matter—a thing. You are more than welcome, however, to submit PRs and address your own questions by looking at the sources (they are all right here on GitHub!) and maybe even posting the answers here in Issues so that everyone else could see them.

Pinging top contributors and demanding something shows you feel entitled. Sorry to burst your bubble but you are not entitled to anything. I personally owe you no more responding to issues than you owe me to respond to other Brunch issues here.

If you like Brunch, you are always free to helps us out by tackling issues, bugs, adding features, and doing community work. In fact, that would be really awesome! ❤️🔥

If you want your project to depend on Brunch but you require support, you're more than welcome to pay for commercial-level support (it's right on the page you linked: http://brunch.io/support). Only then can you demand something... and not from all of the top contributors, but from the people that provide you support — Paul or Andriy in this case.

@c0b
Copy link
Author

c0b commented Dec 22, 2016

so back to this question, could you comment more on "is this a problem brunch can't handle right now?" I see this kind of conditional module loading is pretty common

if (process.env.NODE_ENV !== 'production') {
  var ReactPerf = require('react-dom/lib/ReactPerf');
  var ReactTestUtils = require('react-dom/lib/ReactTestUtils');
  ...
}

@paulmillr
Copy link
Contributor

We don't support this. Conditional imports are also unsupported by ECMAScript; with the import syntax.

@c0b c0b changed the title Question: does brunch support conditional require ? Question: does brunch support conditional require ? (use case in react@15.4 above) Dec 25, 2016
@c0b
Copy link
Author

c0b commented Dec 27, 2016

We don't support this. Conditional imports are also unsupported by ECMAScript; with the import syntax.

I guess this would become big problem; since ReactAddonsDOMDependencies.js is a new file since react@15.4; the if (process.env.NODE_ENV !== 'production') { kind of conditions are also pretty common among other packages, it is compile time constant actually, does brunch have plan to support this special case?

https://unpkg.com/react@15.4/lib/ReactAddonsDOMDependencies.js

if (process.env.NODE_ENV !== 'production') {
  var ReactPerf = require('react-dom/lib/ReactPerf');
  var ReactTestUtils = require('react-dom/lib/ReactTestUtils');
  [...]
}

@shvaikalesh
Copy link
Contributor

We do have plans to implement this.

@goshacmd
Copy link
Contributor

Brunch does replace process.env.NODE_ENV with the current value ("development" for a dev build, "production" for a prod build with -p).

The bundle will still end up containing these files, because our analyzer only detects require calls, without special considerations about where they appear. They will not, however, get required because process.env.NODE_ENV will be production.

If you want Brunch to not include these files in the production build, your PR to add that would be appreciated.

@c0b
Copy link
Author

c0b commented Dec 27, 2016

The bundle will still end up containing these files,

do you mean you cannot reproduce the issue here? from the vendor bundle, I am not seeing react-dom/lib/ReactPerf code is contained, caused the runtime error

https://gist.githubusercontent.com/anonymous/832c7ee2c1b7da09d588971c56343a21/raw/c71219b30c87fca637928f289133a7b614899a87/vendor.js

if ('development' !== 'production') {
  var ReactPerf = require('react-dom/lib/ReactPerf');    // runtime error happened here
  var ReactTestUtils = require('react-dom/lib/ReactTestUtils');

  [...]
}
  })();
});

@gaearon
Copy link

gaearon commented Jan 5, 2017

I'm confused. I wanted to fix this on the React side but I can still see the issue even after extracting requires out of the condition:

screen shot 2017-01-05 at 20 00 45

What supports the conclusion that this was caused by a conditional require?

@gaearon
Copy link

gaearon commented Jan 5, 2017

It appears that the error is not caused by the conditional require.
I can reproduce the same issue with unconditional require.

Instead, I was able to fix it by moving requires inside functions:

Before

  var ReactPerf = require('react-dom/lib/ReactPerf');
  var ReactTestUtils = require('react-dom/lib/ReactTestUtils');

  exports.getReactPerf = function () {
    return ReactPerf;
  };

  exports.getReactTestUtils = function () {
    return ReactTestUtils;
  };

After

  exports.getReactPerf = function () {
    var ReactPerf = require('react-dom/lib/ReactPerf');
    return ReactPerf;
  };

  exports.getReactTestUtils = function () {
    var ReactTestUtils = require('react-dom/lib/ReactTestUtils');
    return ReactTestUtils;
  };

So it looks more like an unrelated timing bug.

gaearon added a commit to gaearon/react that referenced this issue Jan 5, 2017
See facebook#8556 and brunch/brunch#1591 (comment) for context.
This appears to be a Brunch bug but we can keep a temporary fix until the next major.
gaearon added a commit to gaearon/react that referenced this issue Jan 5, 2017
See facebook#8556 and brunch/brunch#1591 (comment) for context.
This appears to be a Brunch bug but we can keep a temporary fix until the next major.
@gaearon
Copy link

gaearon commented Jan 5, 2017

I intend to merge a temporary workaround for this in facebook/react#8686 but it's likely we'll remove it in the next major (since this looks to be a Brunch bug rather than a problem with React).

gaearon added a commit to facebook/react that referenced this issue Jan 5, 2017
* Add manual build fixtures

* Inject ReactDOM into ReactWithAddons from ReactWithAddons

We used to read ReactDOM as a global inside ReactAddonsDOMDependenciesUMDShim.
This didn't work in AMD environments such as RequireJS and SystemJS.

Instead, I changed it so that ReactDOM gets injected into ReactWithAddons by ReactDOM itself.
This way we don't have to try to require it (which wouldn't work because AMD doesn't handle circular dependencies well).

This means you have to load ReactDOM first before using ReactDOM-dependent addons, but this was already the case before.

This commit makes all build fixtures pass.

* Memoize ReactDOM to avoid going into require on every access

* Add Brunch fixture

* Inline requires to work around Brunch bug

See #8556 and brunch/brunch#1591 (comment) for context.
This appears to be a Brunch bug but we can keep a temporary fix until the next major.
@c0b
Copy link
Author

c0b commented Jan 6, 2017

right; I believe the problem is in brunch because I tested webpack with react 15.4.1 unchanged that doesn't have the problem.

so it seems like the problem in brunch is not only in conditional branches, but also in all non-top level require brunch can't handle? I searched in the brunch compiled bundle code, don't see the 'react-dom/lib/ReactPerf' code is registered, so at runtime anything that require this code would fail

@gaearon
Copy link

gaearon commented Jan 6, 2017

The problem might be related to a bug in handling circular dependencies or something like this. Otherwise I'm not sure why inlining requires into a function worked around the problem.

@shvaikalesh
Copy link
Contributor

Hey @gaearon, sorry for the trouble & thanks for the hot fix: it will give us time to resolve the issue the right way.

gaearon added a commit to facebook/react that referenced this issue Jan 6, 2017
* Add manual build fixtures

* Inject ReactDOM into ReactWithAddons from ReactWithAddons

We used to read ReactDOM as a global inside ReactAddonsDOMDependenciesUMDShim.
This didn't work in AMD environments such as RequireJS and SystemJS.

Instead, I changed it so that ReactDOM gets injected into ReactWithAddons by ReactDOM itself.
This way we don't have to try to require it (which wouldn't work because AMD doesn't handle circular dependencies well).

This means you have to load ReactDOM first before using ReactDOM-dependent addons, but this was already the case before.

This commit makes all build fixtures pass.

* Memoize ReactDOM to avoid going into require on every access

* Add Brunch fixture

* Inline requires to work around Brunch bug

See #8556 and brunch/brunch#1591 (comment) for context.
This appears to be a Brunch bug but we can keep a temporary fix until the next major.

(cherry picked from commit ca2c71c)
@gaearon
Copy link

gaearon commented Jan 6, 2017

In the meantime, this should be fixed after updating all React packages to 15.4.2.

@curgery
Copy link

curgery commented Nov 23, 2017

The conditional cast to the window object necessary?
e.g. -
import Perf from 'react-addons-perf';
if (typeof window !== 'undefined') {
window.Perf = Perf;
}

@brunch brunch locked and limited conversation to collaborators Aug 7, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
Development

No branches or pull requests

6 participants