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

Stream is not defined in inherits call. #237

Closed
bushev opened this issue Oct 6, 2016 · 37 comments
Closed

Stream is not defined in inherits call. #237

bushev opened this issue Oct 6, 2016 · 37 comments

Comments

@bushev
Copy link

bushev commented Oct 6, 2016

Hello everyone!

I got the following error on my application bootstrap (Ionic 2 RC):

inherits_browser.js:9 Uncaught TypeError: Cannot read property 'prototype' of undefined

at Object.inherits (inherits_browser.js:5)
    at _stream_writable.js:46
    at main.dev.ts:5

It seems the second argument superCtor that actually a Stream is undefined.

@mcollina
Copy link
Member

mcollina commented Oct 6, 2016

Are you using browserify or webpack? I am not familiar with the Ionic stack.

@bushev
Copy link
Author

bushev commented Oct 6, 2016

@mcollina It uses rollup.

UPD: default config

@calvinmetcalf
Copy link
Contributor

don't use readable stream with rollup, it doesn't work because rollup doesn't support all of commonjs modules, use the node builtins package and just require streams

@ericgundrum
Copy link

ericgundrum commented Feb 22, 2017

This problem also exists with webpack. You can experience it with pouch-websocket-sync-example v0.1.0.

A way around it is to import "stream-browserify" before any other code attempts to import "stream".

@calvinmetcalf
Copy link
Contributor

ok @bushev I wrote a plugin for rollup that fixes this including an artisanal hand port of readable-streams called rollup-plugin-node-builtins that should fix your stuff

@ericgundrum I'm assuming your talking about webpack 2 in which case you probably want to open an issue there about compatibility with commonjs modules (fun fact I didn't invent this trick, others used it before me) and in the mean time if you can't exclude streams from the bundle like in react-native you should just require('streams') somewhere.

@ericgundrum
Copy link

ericgundrum commented Feb 23, 2017

@calvinmetcalf, yes, this is in webpack 2. I worked around the problem by importing 'stream-browserify' before anything that requires streams. I added comments to their old issue webpack#272. Although it was closed some time ago, it still gets comments.

webpack now has a plugin that will detect dependency cycles during compilation. That can help locate the cycle, but you've got to know you have one before you would include the plugin.

I was wondering if this could be fixed in readable-streams by having it check the validity of the object it gets back from require instead of merely catching a throw. Code checking that it has what it needs when there is a chance it doesn't seems like good practice to me.

In the webpack case, the returned object is totally empty. If I recall their code correctly, they begin with an empty object. Then, while initializing it, the second require(stream) happens. They just return the empty object at that point. Because the object isn't initialized, readable-streams can't use it but doesn't recognize that it is broken (as it would with a throw from require).

Because of this failure, the object never gets initialized and webpack reports an error trying to access properties of it. Unfortunately the error message does not indicate which module is having the problem.

Certainly webpack could improve on this and benefit other situations. But I think readable-streams could fix this, too -- at least for any loader that returns an object missing the apis it needs.

@calvinmetcalf
Copy link
Contributor

ok i think this is a slightly different issue, we intentionally DON'T require streams for this reason and because it is a large package that doesn't add anything, unfortunately there are people who do instanceof checks on streams so we only actually required streams if somebody else has done so already, that require('stre' + 'am') syntax will not include the module in the bundle but it will try to get it at compile time and throw an error if it can't.

in other words there shouldn't be a real circular dependency what it sounds like the issue is is that webpack2 isn't actually throwing a run time error when it can't load the package but instead returning an empty object?

@ericgundrum
Copy link

@calvinmetcalf wrote:

in other words there shouldn't be a real circular dependency what it sounds like the issue is is that webpack2 isn't actually throwing a run time error when it can't load the package but instead returning an empty object?

That is what I saw when tracing the code runtime in pouch-websocket-sync-example. But I don't know CommonJS well enough to say the webpack2 behavior is wrong.

In the websocket app, if I recall correctly, the first encounter is when 'pouch-remote-stream/stream' calls require(stream) to make a pair of streams. That require goes thru 'node-libs-broswer' to 'stream-browserify' and on to `readable-stream'.

As webpack2 is processing its first encounter with require(stream) it hits another one. As far as webpack2 is concerned, it has already begun initializing the module, so it is returning what it has for the second request. That just happens to be an empty object. If this is incorrect behavior, then it seems the error is in deciding when a module has truly begun initialization. Or, maybe there are subtleties I don't understand with what 'node-libs-browser' is doing and how webpack2 implements that.

Calvin, I appreciate your having another look at this. If there is anything I can do to help, please ask.

@calvinmetcalf
Copy link
Contributor

calvinmetcalf commented Feb 23, 2017

first off is it undefined or an empty object, the error you posted seams to suggest undefined

by the time stream-browserify requires readable stream it's already exported a function, so what webpack2 should have at this point is that function and everything should be fine, but since it apparently is undefined (like in es6 modules) you're getting that error (as opposed to CJS modules if there was a true circular dependency would have an empty object literal which has a prototype so wouldn't give that error)

@ericgundrum
Copy link

Maybe some of my information is incorrect or incomplete. I am retracing it now. This is what I find...

Uncaught TypeError: Object prototype may only be an Object or null: undefined
    at Function.create (<anonymous>)
    at inherits (inherits_browser.js:5)

Line 5 is

    ctor.prototype = Object.create(superCtor.prototype, {

And superCtor has __proto__ but no prototype property, or any others for that matter.

Looking up the stack, this call comes from inherits(Stream, TransformStream) at line 45 of pouch-stream-server/stream.js

The rest of the stack is just a straight dependency chain -- no cycle. But all this is a symptom, not where the problem originates. I think I need to go look at what happens when 'readable-stream' loads. I'll add more here when I get that.

@calvinmetcalf
Copy link
Contributor

ok I was forgetting that superCtor.prototype was what the error was about not superCtor, but that doesn't change that in CJS that would be a function which would have a prototype and that would be the issue, can you post the code that generates this, like all the webpack config and stuff so I can investigate more ?

@ericgundrum
Copy link

The quickest way for you to get into this is to run pouch-websocket-sync-example. I'm pretty sure all you need to do is clone it, npm install and node server. Then take your browser to http://localhost:3000/. It should hit the error before drawing anything. I don't think there are any other issues that get in the way of seeing this error. (It is the first thing I've fixed in my local repo.)

@ericgundrum
Copy link

ericgundrum commented Feb 23, 2017

I set a breakpoint at Stream = require('st' + 'ream') of '_stream_readable.js'. Stepping over that, Stream looks good, except Stream.Transform is an empty object. I expected it to be a function.

Then, following the creation of 'readable-stream/transform', Transform and sister objects are present, but Duplex.super.Readable.Stream.Transform is an empty object. The sisters to the empty object, such as Duplex.super.Readable.Stream.Writable are not empty.

@calvinmetcalf
Copy link
Contributor

here's something even weirder, I set a breakpoint here and exports.Transform was defined, then set one here After changing the code to be

var stream = require('stream');
var TransformStream = stream.Transform;

and stream.Transform was NOT defined there

that being said just changing the line to

var TransformStream = require('readable-stream').Transform;

fixed it

@calvinmetcalf
Copy link
Contributor

even more bizzarly transform is being executed due to a different require so it should be defined

@calvinmetcalf
Copy link
Contributor

wait but since its a module exports its not getting redefined, but it's just bizzare that it's not being executed at the correct spot

@ericgundrum
Copy link

There seems to be something not right about how Stream.Transform is being created. I remember being in that code when I traced through this problem two weeks ago. I just haven't yet honed in on it. I think it goes bad when, as readable-stream is being instantiated, it creates its transform property. But duplex is in the mix, too.

@ericgundrum
Copy link

ericgundrum commented Feb 23, 2017

I think I see what's happening now. The first require of stream goes something like this..

'websocket-stream/stream.js' -> 'through2' -> 'readable-stream/transform' -> 'readable-stream/lib/_stream_transform.js' -> './_stream_duplex' -> './_stream_readable' -> 'stream-browserify/index.js' -> 'readable-stream/readable.js' -> './lib/_stream_transform.js'

Then 'stream-browserify/index.js' calls 'writable.js' and ''duplex.js'. All looks good so far, until
'stream-browserify/index.js' calls 'readable-stream/transform.js'.

Note that 'readable-stream/transform.js' still is on the stack. Its formation has lead to this call to 'stream-browserify'. At this point, when webpack resolves this second require it returns the unfinished (empty) object it created with the first call to 'readable-stream/transform.js'. As far as webpack is concerned this object is valid and it does a quick return of the empty module.exports object.

This test is right at the top of function __webpack_require__(moduleId). Probably you could get to the problem quickly with a breakpoint there. Then conditionalize it when you determine the moduleId for 'readable-stream/transform.js'.

All this unwinds without complaint, but that empty object sits in the webpack cache. Anything that tries to use 'readable-stream/transform.js' will hit it.

@calvinmetcalf
Copy link
Contributor

when webpack resolves this second require it returns the unfinished (empty) object it returns the unfinished (empty) object

this is the bug in webpack, since it intentionally situates the require call AFTER exporting the function it shouldn't be a problem because dependencies are resolved when they are reached in code

@calvinmetcalf
Copy link
Contributor

this is definitly a bug in webpack because I was able to build it with browserify, I'll post a sumery there

@mcollina
Copy link
Member

I think this is a good issue for the next wg meeting, e.g. how to improve the consumption of this module in bundlers.

@calvinmetcalf
Copy link
Contributor

strictly speaking this module works fine, it's the browserify stream that causes this error but only when imported after through2 and only in webpack

@ericgundrum
Copy link

@calvinmetcalf, thanks for all your work on this.

The problem is set up in through2 by its require('readable-stream/transform'). I reproduced it in your test repo removing through2 and adding require('readable-stream/transform').

@calvinmetcalf
Copy link
Contributor

@ericgundrum yeah I just realized the same thing and updated the repo

ericgundrum pushed a commit to ericgundrum/pouch-websocket-sync-example that referenced this issue Feb 27, 2017
Import 'pouch-websocket-sync' leads to requiring 'stream'.
In the browser, 'stream' is supplied by 'node-libs-browser'
requiring 'stream-browserify'.

Unfortunately, 'stream-browserify' ultimately requires 'stream' again.
(This is to inherit from an exiting definition when one is present.)
When webpack encounters this circle, it eventually returns an empty object,
supplying an unfulfilled dependency.
This is how CommonJS is supposed to work when encountering a cycle.
(Node would behave the same if 'stream' were not built in.)

Importing 'stream-browserify' early, before any module requires 'stream',
creates the base modules before they can get caught in a dependency cycle.
This way those module are present when other modules try to import 'stream'.

More details of this cycle problem are recorded in [readable-stream issue 237]
(nodejs/readable-stream#237) and
[webpack issue 4378](webpack/webpack#4378).
The behavior of webpack in this scenario appears to be as expected.
@calvinmetcalf
Copy link
Contributor

we have now removed all of the too clever stream hacks in the browser version of readable-stream so this shouldn't be an issue anymore

ericgundrum pushed a commit to ericgundrum/pouch-websocket-sync-example that referenced this issue May 3, 2017
Import 'pouch-websocket-sync' leads to requiring 'stream'.
In the browser, 'stream' is supplied by 'node-libs-browser'
requiring 'stream-browserify'.

Unfortunately, 'stream-browserify' ultimately requires 'stream' again.
(Presumably this is to inherit from a built-in definition when the
runtime environment provides one.)
When webpack encounters this circle, it eventually returns an empty object,
supplying an incomplete dependency.
This appears to be how CommonJS is supposed to work.

Here the circle is avoided by importing 'stream-browserify' before
'node-libs-browser' closes the circle on 'stream'.

Importing 'stream-browserify' early effectively imports it with a
smaller context where 'stream' is undefined.
The 'readable-stream' code is written to handle that case, knowing that
'stream-browserify' actually is providing that definition in some cases.

This problem seems similar to one reported as [readable-stream issue 237]
(nodejs/readable-stream#237).
That was closed, indicating user error.

[webpack issue 272](webpack/webpack#272)
has info from many others encountering problems with circular dependencies
in webpack.

[webpack issue 631](webpack/webpack#631)
contains a higher level discussion of how the webpack behavior
causes hard to identify runtime failures.
It also indicates that webpack behaves the same as node,
with many folks calling for some sort of warning.
The issue was closed with reference to the [circular-dependency-plugin]
(https://github.com/aackerman/circular-dependency-plugin).

The stack overflow article [circular imports with webpack returning empty object]
(http://stackoverflow.com/questions/30378226/circular-imports-with-webpack-returning-empty-object)
indicates that this behavior is part of CommonJS.

None of the webpack references mention 'stream' as a
source of the circular dependency.
In fact, they imply that this is not an unusual problem.
@ericgundrum
Copy link

ericgundrum commented May 3, 2017

@calvinmetcalf, thanks for your follow thru. I no longer see the issue as far as I can tell with my limited testing using readable-stream v2.2.9.

@binarykitchen
Copy link

well, this error still happens to me and is not resolved @calvinmetcalf
selection_009

to reproduce, just git clone https://github.com/binarykitchen/videomail-client/tree/feature/rollupjs and run yarn examples...

@mcollina
Copy link
Member

Is this specific to rollup?

@binarykitchen
Copy link

yes, rollup ... but hard to say, could be related, i dunno know

/*<replacement>*/
var util = require('core-util-is');
util.inherits = require('inherits');
/*</replacement>*/

var Readable = require('./_stream_readable');
var Writable = require('./_stream_writable');

util.inherits(Duplex, Readable); // <-------------- triggered here

@mcollina
Copy link
Member

As far as I know, it works ok browserify and webpack. So, it should be a rollupjs problem.
Can you please check if it was working on version 2.2.x?

@binarykitchen
Copy link

how can i switch to version 2.2.x?

@calvinmetcalf
Copy link
Contributor

rollup is NOT like browserify and webpack so readable stream probably won't work due to its' circular dependencies, that's why I wrote rollup-plugin-node-builtins which includes and ES6ified version of streams that should work.

@binarykitchen
Copy link

yeah i am using that plugin already. so what now?

@calvinmetcalf
Copy link
Contributor

see my message in the other issue (calvinmetcalf/rollup-plugin-node-builtins#31), it looks like you are not using it

@CMCDragonkai
Copy link

I don't think this is resolved for Rollup.

@yarra2296
Copy link

@ericgundrum, How to solve the Issue. I am using Webpack 2. I am getting the same Error with React-native. can I know the workaround if any?

@ericgundrum
Copy link

@yarra2296, I think the commit comment on my pouch-websocket-sync-example describes it best.

The simple answer is to import 'stream-browserify' as early as possible. But note from earlier in this thread that Calvin fixed the problem in 'stream-browserify'. If you still see a problem, maybe you can investigate why and file a new issue as appropriate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants