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

Proposal: Remove or deprecate auto-integration in Ender 1.0 #166

Open
amccollum opened this issue Oct 21, 2012 · 28 comments
Open

Proposal: Remove or deprecate auto-integration in Ender 1.0 #166

amccollum opened this issue Oct 21, 2012 · 28 comments

Comments

@amccollum
Copy link
Contributor

amccollum commented Oct 21, 2012

I've been doing some analysis of publicly available ender modules in NPM, and I'd like to share some takeaways. The first one is that I think ender should do away with auto-integration. By that, I mean, when there is no bridge specified in the package.json file. In these cases, Ender attempts to auto-integrate the module:

$.ender(module.exports)

The problem with this is that the majority of the time, this is the wrong behavior (judging by modules in the wild). Here are the numbers I found:

  • total ender modules identified: 168
  • modules with no bridge and a main file: 62
  • modules that don't export anything: 10
  • modules that export only a function object: 14
  • modules that have exports, but should be noop: 20
  • modules that might export useful $.ender methods: 18

In every category except the last, or 44 / 62 modules, integration is either doing nothing, or is actually incorrect behavior. This is with a pretty generous interpretation of what might be useful. Most of the modules in the 'should be noop' category are things like native node modules ported to the browser, modules that export generic names like create that will likely clobber existing functions, modules that are using their main file as a bridge, and modules that export single functions but with useless internal objects or constants attached to those functions.

There are only 33 modules that actually specify a noop bridge, when the number should really be more like 80.

I think that it's better to make noop the default and force people to explicitly integrate their modules. It's easy to do and will avoid issues when people just offer modules through Ender without making sure they only export useful $.ender methods.


Note that my methodology might not be perfect, and there is a subjective element in a few cases, so for reference, here are the modules in each category:

No Exports
cookie-monster
cssclass
Davis
ender-twitter-bootstrap
es5
littering
process
reddy
sHistory
sugar

Export Only A Function
asyncify
aug
chainify
clearInterval
clearTimeout
dice-roll
fileup
framejax
houkou
houkou-tweak
konami
loop
setInterval
setTimeout

Should Be noop
assert
Buffer
campusbooks
console
ender-sc
eventhub
events.node
forEachAsync
future
join
JSON
json-storage
json-storage-model
json-tables
kizzy
lateral
path
sequence
url
util

Might Export Useful Methods
ahr2
bowser
categorizr
delayed
ender-deferred
ender-ejs
ender-validator
fidel
jolt
jaaulde-cookies
jsonapi
loadr
md5
navigator
pathjs
postmessage
serialize-form
webnotify


Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@luk-
Copy link

luk- commented Nov 12, 2012

A side effect of this is breaking compatibility with modules that do work with ender, but aren't labeled. In the past, I've successfully used spin.js with ender, but this module has no mention of ender in its package.json. I would hate to see existing non-ender modules that currently work break because of this, but I agree that something should be done to address the current issues it causes as a result of the existing functionality.

Also, tangentially related, support for simple modules that use the module.exports = function () { // code } pattern without a bridge would be welcome. Perhaps these two matters are more related than I think.

@amccollum
Copy link
Contributor Author

Well, since spin.js uses module.exports, it should be available (unless I'm missing something) through a call to require. The issue is whether the exports should automatically be passed to $.ender. In the case of spin.js, it appears to be exporting a single class, which might actually be an example of when this behavior is incorrect and harmful.

The second approach you mention will work if the module itself does what you describe. I don't see how this would work with arbitrary code, though, because there can't be a return statement in the file if it isn't already wrapped in a function, and if it is wrapped, the author probably has their own ideas how this function gets called and the module gets instantiated.

@luk-
Copy link

luk- commented Nov 12, 2012

In that case, I misunderstood your initial post and agree with what you're saying now. If this doesn't affect the require usage, then it's probably a good idea. I tend to be less concerned with attaching to $ and more interested in the require usage personally.

@rvagg
Copy link
Member

rvagg commented Nov 12, 2012

@st-luke sounds like you'll be appreciative of a move to a pure commonjs system with the $ stuff being an optional extra.

@luk-
Copy link

luk- commented Nov 12, 2012

Absolutely. I think the commonjs style require is one of the best parts of the tool.

@ded
Copy link
Member

ded commented Nov 19, 2012

I'm in favor of the require usage as well, which was a main reason I wanted it on the client to begin with (to mimic Node). In reality, the $ extension behavior is really there to please the jQuery audience whereby you can use the ender core, and use a variety of jQuery plugins.

@symmetriq
Copy link

I like this idea in general, but is there any way to re-enable auto-integration in Ender 2.0 for projects that were built around that feature? I'm trying to update the modules in some of my existing libraries, and don't really want to manually re-add all those methods back to $. I could just keep an old version of Ender around, but Ender 2.0 fixed some annoying issues with 1.1 (barf on .DS_Store files, empty output for ender info for libraries built from package.json, etc.), so I'd have to go back to a version prior to 1.1 to sort those out, then manage two separate versions and remember when to use each one, which is a whole lot of harumph for me.

The other issue for me is that most people where I work use jQuery, so maintaining the $ façade does a lot to reduce friction/resistance to custom-built libraries ("why don't we just stick with the standard?"). It's a much easier sell when I can say "look, we can just swap in jQuery instead and pretty much everything still works", but now that's no longer the case. I could manually re-add all the functions to $, but that really seems like the wrong approach to restore functionality that was already there before. I'm fine with auto-integration being off by default, but if it's completely removed I may be stuck choosing between keeping old, stale Ender-built libraries with out-of-date modules or just switching back to jQuery on existing projects. The other alternative is to just update all the actual function calls throughout my code, but I'd still want everything under $ to avoid polluting the global namespace with a bunch of top-level vars for each module. This is something I don't worry about so much in Node, but on the client side, the window namespace is still a minefield.

As an example of when this would be a total pain in the arse, consider a project that uses Backbone (and therefore also Underscore, which has around 100 methods). I guess I could just do something like this for each module (but…ugh):

var _ = require('underscore');
for (f in _) {
  if (_.hasOwnProperty(f) && !$.hasOwnProperty(f)) { $[f] = _[f] }
}

Seems like a lot of work just to get back to something that worked fine before. To me it would make a lot more sense to provide an optional flag to enable auto-integration and leave it off by default rather than just removing it altogether.

@amccollum
Copy link
Contributor Author

I added a new --integrate command line option in ender-js/ender-builder@27a8ffb that allows you to opt-in to the old auto-integration behavior on a module-by-module basis. So, for example, in the case of underscore, you could do:

ender build jeesh underscore --integrate underscore

This would cause all the functions that underscore exports to be defined on the $ object. This option only works if the module doesn't supply its own bridge file. I am hesitant to add a flag that restores the old default behavior because I still think it causes more harm than good for the reasons stated above.

Your other alternative is to do this manually at runtime like so:

$.ender(require('underscore'))

Does this cover the use case you're describing?

@symmetriq
Copy link

Thanks Andrew!

Is there any way to do this with a package.json? (I guess that approach involves the require per module?) Either way, no biggie; I only use a half dozen or so modules in the projects I was trying to update, so it's easy enough to just specify them directly (though getting slightly more complicated as several of the ones I use now need specific versions to retain IE 6-8 support…I'll spare you the sad tale of why I'm still stuck doing that). But it sounds like this'll do the trick. Much appreciated.

I do agree with the general principle of not automatically mashing every module's public methods into $. Just trying to maintain some of my simpler libraries that only include Jeesh + two or three other modules.

@amccollum
Copy link
Contributor Author

Well, from Ender's perspective, it doesn't care if the package was included from the command line or the package.json (or even if it is a dependency of some other module, for that matter), so it would work just fine if you did ender build . --integrate underscore and underscore was a regular dependency, but there is currently no way to add additional options like --integrate to the package.json if that's what you're asking.

You could create a bridge file for the root package that integrated all the packages you wanted (using the $.ender(require('module')) pattern or just adding selected methods), and that might actually be the cleanest approach since it makes it completely clear what you're doing. Maybe that's what you were suggesting you were trying to avoid, though.

@symmetriq
Copy link

Cheers. I'm trying to sort out the new-as-of-yesterday issues with npm (no longer allows self-signed certs) and install the new Ender 2.0.1 (looks like some of the components have been updated to 1.0.2 already; possibly related?). Will take another run at building my old libraries once I've got everything in order here.

@amccollum
Copy link
Contributor Author

Yeah, the update to 1.0.2 was to update Ender's version of NPM, but I missed a version number update in one of the submodules, and now that you can't force-publish, I just updated everything to 2.0.3 (ender) / 1.0.3 (ender-*).

Going up to ender@2.0.3 should resolve most (all?) your issues.

@symmetriq
Copy link

I did get 2.0.2 and the build (w/ --integrate) is working, but was just trying to work out a few other issues.

The --integrate option mostly works, though it won't integrate anything that's installed as a dependency of another package (e.g. --integrate backbone won't also integrate the required underscore methods), and jeesh can't be specified after --integrate, or it'll complain that there's no such module (maybe because it has its own bridge, or maybe because it's really just a collection of modules).

Lemme give this another whirl w/ 2.0.3.

@symmetriq
Copy link

There's still something broken in Backbone after building the library w/ 2.0.3. It looks like Backbone.$ (which should end up being a reference to the global $, probably?) is undefined. The error occurs in setElement. Stack looks like this:

Module.createPackage.backbone._.extend.setElement
Module.createPackage.backbone._.extend._ensureElement
Module.createPackage.backbone.Backbone.View
Module.createPackage.backbone.child
(anonymous) (the main function wrapper in ender[.min].js)

Not that this is the place to troubleshoot the Backbone module, but it's something that works with the old auto-integration of Ender 1.1, and doesn't with --integrate.

@amccollum
Copy link
Contributor Author

2.0.3 had no functional changes, so whatever issues you're having won't be resolved. I'm not sure what problem you're running into with backbone. Running ender build backbone --integrate backbone underscore seems to work for me. Unless you're saying that it should automatically integrate all dependencies, too. I feel like that's not a common enough case to warrant the added complication to support it.

As for having jeesh after --integrate, it seems like you might be getting tripped up by Ender not knowing where one list of packages ends and the next starts. You can do ender build --integrate underscore --packages underscore jeesh (be explicit about --packages instead of implicit) if you want to disambiguate things.

@symmetriq
Copy link

Yeah I've got the integration part sorted out (I think?) by just specifying all the modules after --integrate. There seems to have been some kind of implied reverse link from Backbone to $ that's no longer there though.

@symmetriq
Copy link

If I just add a breakpoint and manually do: Backbone.$ = window.$ before continuing, everything works. Looks like that happened automatically somewhere in my Ender 1.1-built library.

@amccollum
Copy link
Contributor Author

It's an issue of how ender@2.0.x creates the global $ variable. I'll look at finding a fix for this later tonight.

@symmetriq
Copy link

That seems to be the only remaining issue. Really appreciate your time on this. Gonna try rebuilding some other libraries that don't use Backbone.

@amccollum
Copy link
Contributor Author

Actually, it seems like when Backbone finds that exports is defined, it doesn't pass in the $ variable to the module factory function. Maybe this is a recent change to Backbone which is why it wasn't showing up before. Not sure ender can fix this behavior. Though, as you said, it looks like you could create this reference manually by cloning Backbone, creating a bridge and adding it there.

@amccollum
Copy link
Contributor Author

Seems like the change happened when AMD compatibility was added in jashkenas/backbone@ea29bd9

Maybe we can put in a pull request that the $ argument be passed as normal even when exports is defined.

@symmetriq
Copy link

Was just trying to fudge it by setting up Backbone before passing it to $.ender(), but for some reason that results in an infinite loop of .ajax calls (until V8 hits the max stack limit).

Can't quite pin down why I can't just pass the result of require('backbone') to $.ender() when $.ender(require('backbone')) does work. Might be time for lunch.

(Edit: err, I can do that, but if I try to set Backbone.$ = window.$ before the $.ender(Backbone), that's when it does the endless loop.)

@symmetriq
Copy link

Fixing this in the actual Backbone repo seems like the correct solution here.

@symmetriq
Copy link

What works for now is just specifying backbone@1.1.0 when building my library. My bad, this one had nothing to do with Ender. I'll get in touch w/ Jeremy about the Backbone change.

@symmetriq
Copy link

jashkenas/backbone#3031

@symmetriq
Copy link

So the other solution here, for anyone who wants to use Backbone 1.1.1 or newer, is to skip the $ integration just for Backbone and use it with its own separate reference:

var Backbone = require('backbone');
Backbone.$ = $;

Then Backbone.View.extend({...}) -- instead of $.View.extend({...}) -- and so on. I went through and updated one of my projects and found there were only a half dozen places where I had to change $ to Backbone to make this work (and all other integrated modules are still fine).

@symmetriq
Copy link

So I updated one of my projects with an Ender-(2.0.3)-built library. Backbone is now Backbone (as described in my previous post), and it turns out there's only two modules that need to be explicitly integrated, so I just added the $.ender(require('module_name')) lines, and now the library is back to being built with a simple ender build, from package.json. I think I now prefer this approach.

@symmetriq
Copy link

Submitted a fix for Backbone: jashkenas/backbone#3038

The fix is useful whether Backbone is integrated or not. When not integrated, you won't need to manually assign Backbone.$ = $.

I've got it working with integration as well, but the catch is it'll only integrate with --integrate backbone on the command line. It's still not possible to integrate after-the-fact via $.ender(require('backbone')), as there's still an infinite loop when it hits this part:

Backbone.ajax = function() {
  return Backbone.$.ajax.apply(Backbone.$, arguments);
};

For the sake of simplicity when building the library, I will probably just keep using it without integration, but at least it can work both ways for those who want it to.

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

5 participants