Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

npm run build fails for EventEmitter #1023

Closed
godmar opened this issue Nov 9, 2016 · 130 comments
Closed

npm run build fails for EventEmitter #1023

godmar opened this issue Nov 9, 2016 · 130 comments

Comments

@godmar
Copy link

godmar commented Nov 9, 2016

@gaearon asked that I file a separate issue.

How to reproduce: run create-react-app fail-event-emitter, add a line

import EventEmitter from 'events';

to src/index.js and then run npm run build, which fails with:

> [email protected] build /home/gback/fail-event-emitter
> react-scripts build

Creating an optimized production build...
Failed to compile.

static/js/main.068c41be.js from UglifyJs
Unexpected character '`' [/usr/lib/nodejs/events.js:260,0]

react-scripts 0.7.0 is used and node.js 6.9.1 (or 7.0)

The purported cause is that CRA does not transpile "dependencies" into ES5 and then the UglifyJS plugin fails when it encounters ES6 code.
The test case above was derived from a failure in react-bootstrap-tables, which includes a module that in turn references EventEmitter. Others have suggested to provide an alternate EventEmitter implementation in the application path, but it is not clear to me how react-bootstrap-table's dependencies can be made to use this alternate implementation.

@godmar
Copy link
Author

godmar commented Nov 19, 2016

Same issue when including "recharts": "^0.18.0". This package also includes EventEmitter.

It looks as though at this point, the chances of successfully building a 'production' build with create-react-app are rather low.

In related posts, it was mentioned as a work-around to include our own implementation of EventEmitter. Can someone describe how to do that? Specifically, I do not know how to ensure that dependencies such as recharts import the included implementation of EventEmitter rather than falling back onto node.js's. Is there a webpack setting? If so, react-scripts should probably include it.

Would be great if this were addressed. Currently the only work-around is to disable the 'UglifyJSPlugin', but this requires reaching inside CRA (or ejecting).

@henri-hulski
Copy link

We replaced the node EventEmitter with EventEmitter3 for Cerebral2.

But this was only possible because we are the authors of all dependencies which use the EventEmitter.

@gaearon
Copy link
Contributor

gaearon commented Nov 19, 2016

Can you investigate where this problem is coming from? I'm sorry I don't have much time to investigate it right now but I'd be happy to accept a fix for it.

It looks as though at this point, the chances of successfully building a 'production' build with create-react-app are rather low.

Well this problem never occurred before, so it must be something new. Can you take time to figure out why it happens and propose a fix?

@gaearon
Copy link
Contributor

gaearon commented Nov 19, 2016

In particular I would expect Webpack to take EventEmitter implementation from some collection of shims. The question is, then, why those shims aren't in ES5 anymore and where they are located. This is not something you would require CRA maintainers to investigate—you could try to dig into it by yourself and figure out which package is responsible for that code and when it changed.

@thien-do
Copy link
Contributor

thien-do commented Nov 19, 2016

Updated: comment deleted since I'm misunderstand the situation 😄

@jquense
Copy link
Contributor

jquense commented Nov 19, 2016

generally the webpack shims come from: https://github.com/webpack/node-libs-browser assuming something else isn't messing with the module resolution

@gaearon
Copy link
Contributor

gaearon commented Nov 19, 2016

The question is, then, what changed there that the source of EventEmitter is no longer ES5.

@henri-hulski
Copy link

events.js uses template strings from node 6.0.0.
They were introduced in c6656db.

@henri-hulski
Copy link

So it comes from core node.
But not happens on all machines so not really sure why.

@thien-do
Copy link
Contributor

@henri-hulski it seems webpack is supposed to not use that native implementation from NodeJS, but instead use one from node-libs-browser.

@henri-hulski
Copy link

In my case it uses the native implementation.

@henri-hulski
Copy link

And the example in the first post by @godmar gets also exactly the same error message as I from events.js:

Unexpected character '' [/usr/lib/nodejs/events.js:260,0]`

@thien-do
Copy link
Contributor

right, but in my case it used the shim implementation. So there are some difference between our system that cause this issue, I guess?

I'm using Mac OS Sierra, and my Node is 6.3.0, could that be the difference?

@henri-hulski
Copy link

A detailed description of my case you find in cerebral/cerebral#389.

My system:
System: Debian GNU/Linux 8.6 (jessie).
Linux version: 4.7.0-0.bpo.1-amd64
Node version: 6.9.1
npm version: 3.10.8
react-scripts version: 0.7.0

In our team only 2 persons got this bug.
So yeah it depends on the configuration.

@henri-hulski
Copy link

The second configuration which got this error was:

Kernel : Linux 4.4.0-45-generic (x86_64)
Compiled : #66-Ubuntu SMP Wed Oct 19 14:12:37 UTC 2016
C Library : Unknown
Default C Compiler : GNU C Compiler version 5.4.0 20160609 (Ubuntu 5.4.0-6ubuntu1~16.04.2)
Distribution : Linux Lite 3.0

npm: 3.10.8
node: 6.9.1
react-scripts: 0.7.0

@thien-do
Copy link
Contributor

Are all your team using the same version of distribution of Linux? There are people reported that they see similar issues in Ubuntu

@jquense
Copy link
Contributor

jquense commented Nov 19, 2016

have to check with the webpack team I think, but I believe depending on the target it will: include a shim, use the native code, or include nothing. https://webpack.github.io/docs/configuration.html# node the docs would suggest that not all node modules have this behavior but the docs may be incomplete.

newer node will have es6 code in the core implementations so maybe it's folks on newer node? the shims haven't been updated in 6 months so they probably aren't the problem.

@henri-hulski
Copy link

BTW
When fixing /usr/lib/nodejs/events.js, I get similar errors in /usr/lib/nodejs/internal/util.js:

Failed to compile.

static/js/main.c359f549.js from UglifyJs
Unexpected character '`' [/usr/lib/nodejs/internal/util.js:4,0]
Failed to compile.

static/js/main.3335427e.js from UglifyJs
Unexpected character '`' [/usr/lib/nodejs/internal/util.js:30,0]
Failed to compile.

static/js/main.cebb091d.js from UglifyJs
SyntaxError: Unexpected token: punc (.) [/usr/lib/nodejs/internal/util.js:64,0]

And I also got error when using node v5.12.0 before:

static/js/main.957b2071.js from UglifyJs
SyntaxError: Unexpected token: keyword (const) [/usr/lib/nodejs/util.js:564,0]

@henri-hulski
Copy link

@jquense So yeah I think you're right.

@godmar
Copy link
Author

godmar commented Nov 19, 2016

So is there a solution that does not require dropping/commenting out UglifyJS?

@dvkndn mentions shims for webpack - presumably, we need to tell it to transpile events.js into ES5 when loading it --- isn't this fundamentally at odds with @gaearon statement that 'dependencies' are not transpiled?

The offending commit (pointed out by @henri-hulski c6656db352973d6aea24cb1a3c76adf042b25446) is from Jan 20, 2016, and presumably made it into node releases shortly after? That's 10 months (aka an eternity in this line of work ;-))

@henri-hulski
Copy link

@godmar But as shown above I already had problems with node 5.12.0.
So the problem is the webpack configuration.
The target should be web.

@godmar
Copy link
Author

godmar commented Nov 19, 2016

@henri-hulski Where do you add the target?
I tried adding it to node_modules/react-scripts/config/webpack.config.prod.js but no luck.
I tried both as a top-level option as well as inside the output { } section.

@godmar
Copy link
Author

godmar commented Nov 19, 2016

BTW, @henri-hulski node 5.12.0 was released 2016-06-23 so that's consistent with my statement since the commit is from Jan 2016.

Also, why would include NodeSourcePlugin fix the issue; the documentation says that it "adds polyfills for process, console, Buffer and global if used." - but hasn't EventEmitter be moved outside process. in node.js 6.***?

Should we file an issue with webpack and ask them what configuration is necessary? Presumably, it's not ok for webpack to assume that a build can use ES6, even if it went straight to the browser and didn't get caught up in UglifyJS, or is this ok to assume now?

@henri-hulski
Copy link

henri-hulski commented Nov 19, 2016

It should go on top level. But it's actually the default. So not sure what happens here.
See https://webpack.github.io/docs/configuration.html.

@sokra
Copy link

sokra commented Dec 2, 2016

@TheLarkInn This diagram is webpack 2, but CRA uses webpack 1 which uses a different resolver impl.

root is tried before node_modules, fallback is tried after node_modules. It's a bit confusing, that's why it was changed for webpack 2.

@TheLarkInn
Copy link

Ah sorry I totally forgot cra is webpack 1. Apologize for the confusion.

@godmar
Copy link
Author

godmar commented Dec 2, 2016

@sokra - this may sound blasphemous, but are you sure that webpack 1.13.2 works the way you describe? In particular, in the code, you do what I quoted above

The plugins for the directories in 'root' and the plugins for the directories in 'fallback' are simply concatenated (unless modulesDirectories is set, which in CRA it is not).

Once this reaches enhance-resolve, all you have is a list of plugins for each directory in root and each directory in fallback, concatenated - how can you look in 'node_modules' after root and before fallback?


(Correction to this entry: I had overlooked that by not setting modulesDirectories, CRA is relying on the default setting including 'node_modules', see below)

@sokra
Copy link

sokra commented Dec 2, 2016

I tried it (1.13.3) and it does work. The order in which plugins are added is used. They are called in parallel but the results are used in order.

@godmar
Copy link
Author

godmar commented Dec 2, 2016

@sokra - I see. It's done however iff the default setting for moduleDirectories is not overridden. In other words, it relies on the default setting of modulesDirectories being ["web_modules", "node_modules"]. If you set modulesDirectories: [] it won't work.

So if you accept PR #1131 then the search order would be:

  1. paths.ownNodeModules (which is node_modules/react-scripts/node_modules)
  2. web_modules
  3. node_modules
  4. paths.nodePaths (this is where any NODE_PATH settings would come in)

is this correct?

@fson fson modified the milestones: 0.9.0, 0.8.0 Dec 3, 2016
@godmar
Copy link
Author

godmar commented Dec 7, 2016

I have a working theory. If webpack attempts to resolve a module name such as events or timers it will use the node-libs-browser shims if and only if those names are still unresolved after all of root, modulesDirectories, and fallback have been searched.

I'm basing this on these observations:

(1) NodeSourcePlugin (code) calls compiler.resolvers.normal.apply in "after-resolvers"

	compiler.plugin("after-resolvers", function(compiler) {
		var alias = {};
		Object.keys(nodeLibsBrowser).forEach(function(lib) {
			if(options[lib] !== false)
				alias[lib + "$"] = getPathToModule(lib, options[lib]);
		});
		if(Object.keys(alias).length > 0) {
			compiler.resolvers.normal.apply(
				new ModuleAliasPlugin(alias)
			);
		}
	});

(2) WebPackOptionsApply (code)

    compiler.resolvers.normal.apply(
...
        makeRootPlugin("module", options.resolve.root),
        new ModulesInDirectoriesPlugin("module", options.resolve.modulesDirectories),
        makeRootPlugin("module", options.resolve.fallback),
...
    );
...
    compiler.applyPlugins("after-resolvers", compiler);

This would explain why placing a shim manually in node_modules works even on Linux systems that have the built-in modules in NODE_PATH, and it would explain why on Macintoshes that don't set NODE_PATH the browser shim is found as well.

FWIW, the documentation of NodeSourcePlugin refers to it as providing polyfills - which generally means they are meant to be used only as a last resort.

If my theory is correct, then either the nodePaths patch should be reversed, or amended to include warnings if users' NODE_PATH contains the locations of node's built-in modules. On the other hand, we can't really tell if a user's NODE_PATH setting is intentional or not. It could be that a future version of node.js provides a perfectly usable module 'xyz' in /usr/lib/nodejs and the user wants to include it in their build.

Maybe have a separate variable "RESPECT_NODE_PATH" for the use cases where NODE_PATH should be respected, and don't respect it by default.

@gaearon
Copy link
Contributor

gaearon commented Dec 7, 2016

How about we intentionally deviate and only respect relative NODE_PATH. That's what our users use it for anyway. And global is always dangerous.

@fson
Copy link
Contributor

fson commented Dec 7, 2016

Or maybe check that the path is inside the project folder? I think it would be less surprising than not allowing absolute paths, what matters is where the files are located, not how that location is represented (absolute vs relative).

@gaearon
Copy link
Contributor

gaearon commented Dec 7, 2016

I can imagine people using something like ../shared or equivalent. This is in any case a stopgap solution before we remove NODE_PATH support altogether.

@gaearon gaearon modified the milestones: 0.8.2, 0.9.0 Dec 7, 2016
@gaearon
Copy link
Contributor

gaearon commented Dec 7, 2016

Apparently this problem only exists when you install Node from source.
I missed this detail.

@henri-hulski
Copy link

Nope. Not from source but from NodeSource Node.js Binary Distributions.
The recommended way to install recent Node.js versions on Linux is to use setup scripts (like this for Debian / Ubuntu) which set up their repo on the system so Node.js can be installed and updated through the system's package manager.

@gaearon
Copy link
Contributor

gaearon commented Dec 7, 2016

Right. But those distributions do include source, don't they?
For comparison, I can't find source for events.js on my distribution and seems like I'm not alone.

@gaearon
Copy link
Contributor

gaearon commented Dec 7, 2016

Fixed in #1194.

@gaearon gaearon closed this as completed Dec 7, 2016
@henri-hulski
Copy link

@gaearon No it's not the source. In Linux libraries in general install themselves under /usr/lib, /usr/share/lib or similar folders.
Maybe in macOS they use one binary instead.

@gaearon
Copy link
Contributor

gaearon commented Dec 7, 2016

@niksudan
Copy link

niksudan commented Dec 8, 2016

My build now succeeds 👍

@godmar
Copy link
Author

godmar commented Dec 8, 2016

github is really cool. I filed an issue with nodesource/distributions, which now appears cross-linked here. Maybe we can find out why they have this/where it comes from.

Just wanted to add one thing: events.js is also baked into the node executable in Linux. (This is done via its bootstrapping process where they take a snapshot of the heap that subsequent runs load from.) In other words, doing a require('events') does not actually read /usr/lib/nodejs/events.js even on Linux. It's not clear why they include the source, especially since changing the source wouldn't affect executions of node.js.

@gaearon
Copy link
Contributor

gaearon commented Dec 8, 2016

¯\(ツ)

Thanks for all your help and for attempting to dig to the very root of the issue too.

@nikhil2882
Copy link

nikhil2882 commented Sep 21, 2018

In my case, I was getting unable to minify ./node_modules/EventEmitter/src/index.js:16 and after a lot of research problem was still there. so at the end, I went to babel playground and converted the whole file to es5 and paste there and it worked, don't know why but it worked

@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests