Cannot define singletons

[mockdeep]

Came across this issue yesterday. We're transitioning our app gradually to browserify, so we've got places in our code that have sprockets requires and other places that we're doing browserify requires. (No more places where we've got sprockets requires and module.exports in the same file, though.) The issue seems to be that when we have a sprockets require of a file that itself has a node style require, the node style require gets re-run. A slightly editorialized example, so please forgive inconsistencies:

// ----------------- testy.js ---------------- //
var TestyThing = function () {
  this.stuff = 0;
  console.log('initting my stuff');

  this.addStuff = function (newStuff) {
    this.stuff += newStuff;
    console.log('added some stuff, now I have: ', this.stuff);
  };
};

module.exports = new TestyThing();

// ----------------- other_file.js ---------------- //
var testyThing = require('testy');
testyThing.addStuff(6);

// ---------------- application.js ----------------- //
//= require self
//= require other_file

var testyThing = require('testy');
testyThing.addStuff(5);

Output:

We've been seeing a pretty massive performance impact due to our transition, ~5 minutes to build, and I'm thinking maybe this has something to do with it. We're requiring underscore in a handful of places, so if that is having to be re-evaluated each time then it could be pretty costly on its own. We'll probably end up having to assign things to globals for the time being to reduce the performance hit until we can get rid of all of our sprockets requires.

[cymen]

I'll think about this some more but I think your workaround (going to globals while the singleton is in both places) is a good solution. It's annoying but Sprockets hands us a stream of JavaScript and browserify is run on it. Apparently, the module identifiers are different for the different code paths when pulling in the singleton. I don't know if there is a way to have browserify use consistent identifiers -- ie based on the content instead of just assigning a numeric id. It suspect a solution would require overriding the identifier to be based on the file path. I believe it is technically possible but might be a fair amount of work to implement.

[cymen]

Oh, and is the build time going up 5 minutes due to using browserify-rails? If so, are you using browserify-incremental? My recommendation would be use this gem to get to CommonJS and then jump off to browserify or webpack building directly. It'll give you better performance.

[cymen]

Wait -- so I think this problem is basically external and internal -- if you make one JS bundle with the singletons marked internal and then mark them as external on any other bundle, it should work correctly. At least in browserify land. I think it's possible to bring that configuration over to this gem.

[cymen]

See this example: https://github.com/substack/node-browserify#external-requires

So might not be external but basically, there is a way to tell browserify not to resolve given module internally (so not to bundle it -- to rely on an external bundling). Getting that working would solve your singleton problem (although if there are many, it may be a pain).

[mockdeep]

We're using the default settings, which I think uses browserify-incremental, doesn't it? At least, we're not doing anything to disable it...

I'm going to have to educate myself a little on the internal vs external bundling. To the best of my understanding this is all within the same bundle, or does using sprockets requires end up causing multiple bundles to be built separately?

[cymen]

You're ending up with one asset file however when you take multiple dives from sprockets JS -> CommonJS, you're ending up with separate "worlds" of modules. So when the singleton module is resolved in one "world", it's not the same as the other "worlds" (if the dives were separate). A world is really a JavaScript scope in the end. By using these other settings, you can tell browserify a module should be resolved outside of the "world" -- at a more global scope.

So the sprocket requires do indeed cause something like multiple bundles but due to being in the asset pipeline, unless you explicitly configure another bundle, they'll all end up in the same file.

[mockdeep]

Makes sense, thanks for the lesson. A lot of stuff is global in the current application already, so might stick with that approach for the time being and start scoping it down after we get rid of the sprockets stuff. We've been localizing most of the stuff as we go, but this throws a wrench in the process.

[cymen]

Yeah, I can understand that. I put a warning in the README a while back but I didn't realize the pattern we're talking about would break singletons until you pointed it out. So now I have a more concrete reason why it is best to have one jump from Sprockets -> CommonJS.

I'll keep thinking about this. It's a thorny issue.

[PikachuEXE]

TL;DR

Any summary on possible workaround?

[mockdeep]

We ended up keeping things that needed this behavior on the global scope until we were able to remove all of the sprockets requires.

[PikachuEXE]

@mockdeep
Thank you

Wish there is a way to use require()...

[mockdeep]

@PikachuEXE you can still use require(), but you need to do some workaround to make it work until you get rid of all the sprockets requires. In a lot of cases you may already be assigning things on the global scope if you're using //= requires, so those can actually just change immediately to require(). You could do a couple of things, such as:

// application.js
require('my_stuff');

// my_stuff.js
window.my_stuff = window.my_stuff || { my: 'stuff' };
module.exports = window.my_stuff;

Alternatively, you can do something like:

// application.js
window.my_stuff = require('my_stuff');
// always refer to `window.my_stuff` in your code

// my_stuff.js
module.exports = { my: 'stuff' };

[PikachuEXE]

@mockdeep
Thanks
We also want to use import/require for external lib
But ends-up in multiple copies of external lib code in one precompiled file
Still finding a way to workaround it

[mockdeep]

@PikachuEXE ah, yeah. We ran into that, too. We ended up assigning all of those to the global scope as well:

// application.js
window.underscore = require('underscore');

// elsewhere.js
var _ = window.underscore;

We did it this way for a couple of reasons, rather than assigning window._. 1) It was easier to find and replace later when we did remove the global, and 2) we were finding sometimes window._ would get clobbered by something else on the global scope, presumably a 3rd party plugin or bookmarklet in the user's browser or a virus.

[PikachuEXE]

The best case we think: (if this issue doesn't exist)

// application.js
//= require_tree ./some_component

// components/some_component.es6
const _ = require('underscore');

So now we should have something like this?

// application.js
//= require_tree ./dependencies
//= require_tree ./some_component

// dependencies/underscore.js
window.underscore = require('underscore');

// components/some_component.es6
const _ = window.underscore;
// so stuff

Just not sure how to specify the dependencies easily for different components

[mockdeep]

That looks like it'll work so that your dependencies are loaded first. Otherwise, you can add them to application.js and do a //= require_self as the first require.

One piece of advice I would give, though, is get rid of all require_trees and require files directly. That'll make it easier to transition files one-by-one, keep track of the count of files left, and probably most importantly, make sure you're not incidentally requiring them both with sprockets and with browserify. E.g.:

// application.js
//= require ./dependencies/underscore
//= require ./components/some_component

// dependencies/underscore.js
window.underscore = require('underscore');

// components/some_component.es6
const _ = window.underscore;
// so stuff

We actually wrote a script to automate some of the conversions and check them in a handful at a time. I'm not sure how helpful it'll be, since it's pretty specific to our setup, but I added a gist here. Chk. is the global namespace we had things assigned to before, so the script looks for that and replaces it with require() and module.exports.

[mockdeep]

(Slightly updated snippet in comment, so you may want to take another look.)