Warn about cyclic/circular requires

[dizzib]

Would be useful if browserify could detect this scenario and issue a warning or error. This is because I'm trying to avoid such dubious scenarios in my code.

foo.js require('./bar');
bar.js require('./baz');
baz.js require('./foo');

btw thanks for such a great tool!

[ibc]

No, that's not an error at all, and that happens many times in tons of Node code out there.

• http://nodejs.org/api/modules.html#modules_cycles
• http://selfcontained.us/2012/05/08/node-js-circular-dependencies/

Please somebody close this bug.

[dizzib]

Yes I know it's not an error, just that I'd (sometimes) like to be notified when such a situation occurs as it often indicates my code can be somewhat improved.

[ibc]

Well, I don't think it should be warned as it is a desired design in many cases. Why to warn about something that is just correct?

[dizzib]

I'm talking about my own browser-side codebase here not node. I'm deeming that CDs are incorrect in my codebase even though they may be acceptable in other codebases.

This has nothing to do with node.

[Macil]

Could this check be done as a separate transform or browserify plugin?

[flying-sheep]

I spent quite some time today debugging my application, as I didn't think of cyclic dependencies being a problem.

1. Why is this property undefined?
2. It's clearly not undefined in other places, something has to unset it
3. After analysing the whole data flow: Nope, nothing in my code can do this
4. Nope, the frameworks I use don't do it either
5. OK, I know where it is undefined now, but since nothing unsets it, ...???
6. The whole object is empty?
7. It's already empty right after requiring it?? Wtf?

Only then the possibility of cyclic dependencies dawned on me. Fuuuuuu

So yes, warnings by default would be nice. It would save people time and hint at the existence of the possibility that things can fail, instead of silently failing. Nothing keeps people from turning them off.

And please only warn for own things, not only these in node_modules, as apparently it's common there.

[ibc]

"Empty object" is not the only problemas with cyclic dependencies, but just one of them, so there is no easy way to "warn" the user. Just export at the top of your file and you are done.

[feross]

Cyclic dependencies can happen in node too, and they don't warn about it.

On Mon Dec 08 2014 at 11:14:10 AM Iñaki Baz Castillo <
notifications@github.com> wrote:

"Empty object" is not the only problemas with cyclic dependencies, but
just one of them, so there is no easy way to "warn" the user. Just export
at the top of your file and you are done.

—
Reply to this email directly or view it on GitHub
#961 (comment)
.

[flying-sheep]

Just export at the top of your file and you are done.

you mean export, then put require() calls? that’s pretty bad for readability. i’ll have to scroll to the bottom to see things a file requires on.

Cyclic dependencies can happen in node too, and they don't warn about it.

i’m pretty sure that i’ve never encountered any environment where something like this silently fails by returning a valid object of any kind. failing stuff should throw an exception, or at least return undefined, null, python’s None, Option::None, …

if it would have been undefined instead of {} i’d instantly have spotted it:

1. what do you mean, ServerParamStore is undefined?
2. it’s directly returned by require, so it must be something require does
3. <realization>

[ibc]

you mean export, then put require() calls? that’s pretty bad for readability. i’ll have to scroll to the bottom to see things a file requires on.

No, it is a good design pattern and 100% readable.

i’m pretty sure that i’ve never encountered any environment where something like this silently fails by returning a valid object of any kind. failing stuff should throw an exception, or at least return undefined, null, python’s None, Option::None, …

You don't get the point. module.exports does exist by default and it is an empty object unless you override it (which it is done when you do module.exports = something).

if it would have been undefined instead of {} i’d instantly have spotted it

Tell Node and CommonJS to change it. Get the point?

[flying-sheep]

you could stop being so condescending as a first step, then i’ll be more inclined to listen to your arguments.

No, it is a good design pattern and 100% readable.

where you put your import statements isn’t a “design pattern” it’s style. unless the order and position of the import statements matter, then it can be a workaround to the problems arising by a badly designed import methodology (which it is due to the order and position of the require() calls mattering.)

browserify relies on static analysis. it might as well use it to improve user experience.

module.exports does exist by default and it is an empty object unless you override it

sure, not that it would be of any use to import something that doesn’t attach to or override it (yes, i know you can require() something for its side effects but if anything is bad design, then that!).

so let’s warn users if a module wanting to override it doesn’t get to do it (is attached to an object by require() before being executed), and if a required thing never attaches to/replaces module.exports.

---

so i have code that makes browserify execute one module (a) without having executed a module (b) it require()s. and worse, even after it ran for a while, the object in module (a) that should be module (b) is still empty.

this led to a bug that cost me 1½-2 hours of my life.

i’d like to prevent others from suffering in the same way, and due to it not being resolved later, browserify has to know* that it failed irrevocably at one point. there is a code branch somewhere that says “fuck it, let’s just return module.exports even though the module hasn’t been executed”

i want it to tell us if it does that. that’s reasonable, i’m sure.

[ibc]

you could stop being so condescending as a first step, then i’ll be more inclined to listen to your arguments.

It was not my intention. Sorry for that.

No, it is a good design pattern and 100% readable.

where you put your import statements isn’t a “design pattern” it’s style. unless the order and position of the import statements matter, then it can be a workaround to the problems arising by a badly designed import methodology (which it is due to the order and position of the require() calls mattering.)

You know that in C/C++, if you #include headers in the bad order you get similar issues.

Let me tell my experience and the rationale under my opinion:

Recently I "migrated" JsSIP (which is a very complex JS library) from a "legacy" design (using concat to generate a final build) to a Node-like project. During the transition I run into the "cyclic dependencies" problem many times. It was not just about getting an "empty object" but, sometimes, I got wrong behavior due the usage of a non completely "exported" module (for example, A.js exports an object with keys a, b, and c, but at the time it is "required" by other module c was not yet set).

The solution to ALL my problems was what I suggest in this issue:

• Give a value to module.exports (I never use the exports reference alone) first, and complete it later.

This is:

• If a module/file must exports a "class" (a constructor) then do this:

module.exports = MyClass;

var dep1 = require('./dep1');
var dep2 = require('./dep2');
var dep3 = require('./dep3');

function MyClass(foo, bar) {
  [...]
}

MyClass.prototype.hello = function() {
  [...]
};

• If a module/file must exports an object then do this:

var exported_object = module.exports = {};

var dep1 = require('./dep1');
var dep2 = require('./dep2');
var dep3 = require('./dep3');

exported_object.foo = function() {
  [...]
};

exported_object.bar = 'BAR';

[...]

browserify relies on static analysis

So Node does, regardless it does it in runtime (the mechanism is the same).

it might as well use it to improve user experience.

By adding something that works differently in Node and browserify?

so let’s warn users if a module wanting to override it doesn’t get to do it (is attached to an object by require() before being executed), and if a required thing never attaches to/replaces module.exports.

That is so hard considering that you must deal not just with module.exports but also with exports (note that CommonJS just defines exports object, while Node added module.exports so you can export stuff different than an object). I cannot figure out how browserify can check that a file does not override the reference to exports once others have already required it. Also note that adding stuff to an object once another module has load it should not cause wrong effect if the required object field is not read before runtime. Example:

• file A.js:

var exported_object = module.exports = {};

var B = require('./B.js');  // We load B, which loads A before A has been *completed*.

exported_object.foo = 'FOO';
exported_object.bar = 'BAR';

• file B.js:

module.exports = MyClass;

var A = require('./A.js');

function MyClass() {
  this.foo = A.foo;  // NO PROBLEM since A.foo DOES exist in runtime once B.js
                             // has been load.
}

so i have code that makes browserify execute one module (a) without having executed a module (b) it require()s. and worse, even after it ran for a while, the object in module (a) that should be module (b) is still empty.
this led to a bug that cost me 1½-2 hours of my life.

I insist (sorry) with the above "style". No issues since I use it.

i’d like to prevent others from suffering in the same way,

You would prevent them when using browserify but they would suffer it when using Node. That's why I see no point in your proposal.

and due to it not being resolved later, browserify has to know* that it failed irrevocably at one point.

As stated above it is 99% hard to determine when something will fail or not, and when it should warn or not (check my latest example of A.js and B.js).

[flying-sheep]

It was not my intention. Sorry for that.

thank you. the two uses of “did you get the point” were a bit off-putting.

You know that in C/C++, if you #include headers in the bad order you get similar issues.

yes, because including files is a ridiculously bad import mechanism :)

• If a module/file must exports a "class" (a constructor) then do this

unfortunately, they are filled with React.createClass({ dozens of lines of stuff }) or Reflux.createStore({ dozens of lines of stuff })

I cannot figure out how browserify can check that a file does not override the reference to exports once others have already required it

hmm, i think the problem is the pre-fetching: A replaces module.exports, and B replaces its own as well.

so i have the following, i think:

• file A.js:

var B = require('./B.js') //we now switch to loading B.js
module.exports = Reflux.createStore({
    dozens of lines of stuff, using B
})

• file B.js:

var A = require('./A.js') // since A started to be executed, we don’t switch to A now but
            // use the predefined module.exports, even if it gets replaced in the next line.
module.exports = Reflux.createStore({
    dozens of lines of stuff, using A
})

i don’t think i can solve this without putting the requires on the bottom of the file, after a screen of other stuff.

[ibc]

Your use case is too complex IMHO. You are exporting the return value of a function, so indeed you need to have the whole A loaded before the module.exports line in B. I'd just avoid that.

Export a function that you must call instead of exporting the result of such a function. The difference is that by having to call such a function its body is processed in runtime (once the dependencies are fully loaded).

BTW your example above is just unfeasible.

[flying-sheep]

BTW your example above is just unfeasible

well no. the stores just listen to each other and update their state from each other when certain events occur:

1. serverResponseStore replaces its params field using the one from submittedParamsStore as soon as the server responds (to have a reference to the last successfully submitted params)
2. submittedParamsStore takes its params field from serverResponseStore if the server calculation is cancelled (i.e. revert to the last successfully submitted params)