Multiple backbones inside Node makes instanceof fail #1288

Closed
balupton opened this Issue May 7, 2012 · 16 comments

7 participants

@balupton

This just consumed the last 4 hours of my life... and boy was it the most crazy difficult thing to debug.

If we have two modules:

  • Module A provides custom backbone.js models
  • Module B provides custom backbone.js collections

Then we have our app which:

  • Requires Module A
  • Requires Module B
  • Creates a model instance using a custom model from Module A
  • Creates a collection instance using a custom collection from Module B
  • Adds the model instance to the collection instance

Then the sky falls down and burns all the horrible plagues at once. Why? I know the answer, but I'll give you a gold star if you can guess it.

Here's the answer: because our lovely model instanceof Model inside Backbone.Model.prototype._prepareModel fails, as the models are using one backbone.js and the collections are using another!!!! Despite them being the same backbone.js version.

ARGH! That line needs to be better, perhaps check for the presence of a this.isBackboneModel variable defined by the Backbone.Model.prototype.

ARGHHHH. For now, it seems the only solution is to pass the Modules the Backbone.js module that we want them to use, rather than having them use their own local copies. Which is a real PIA. Which imho, the flag idea is a much better one.

Thoughts?

@yuchi

Duck typing FTW.

@tbranyen
Collaborator

How are you getting two different versions of Backbone? Node internally caches the loaded exports object and will use that exact same reference whenever you use require.

Can you push up your code to a repo for further inspection?

@japj

I think nodejs caches the loaded export based on the absolute filename of the 'exported object'.
Can it be that you have 2 backbone.js files located on your filesystem (one in each "module" directory)?

@yuchi

What could lead this is simply two different required versions:

├─┬ awsome-package@x.x.x
│ └── backbone@0.5.1
└─┬ incredible-package@y.y.y
  └── backbone@0.5.0

Instead of:

├─ awsome-package@x.x.x
├─ incredible-package@y.y.y
└─ backbone@0.5.1

EDIT @balupton said:

Despite them being the same backbone.js version.

@balupton

@yuchi nailed it.

I've built a sample app that highlights the problem: https://github.com/balupton/backbone-instanceof-bug

@balupton

How are you getting two different versions of Backbone? Node internally caches the loaded exports object and will use that exact same reference whenever you use require.

It appears it is due to node.js only internally caching the results of require('backbone') per module, not for your entire application. So for each module, their require('backbone') includes their module's copy of backbone, not the applications.

@tbranyen
Collaborator

You're using package.json very incorrectly. A package.json should exist at the root of your application not your module. Each module having a package.json does not make sense, since the definitions therein define the package as an application. Therefore your entire application should have a single node_modules at the root containing a single instance of Backbone.js.

There is nothing Backbone.js can do to solve this problem for you, its entirely a design decision to include two redundant copies of Backbone. There might be a way you can work around the issue, but instead you should try and figure out why you need multiple application configs (package.json) and eliminate those so you can avoid issues like this.

@tbranyen tbranyen closed this May 7, 2012
@balupton

You're using package.json very incorrectly. A package.json should exist at the root of your application not your module. Each module having a package.json does not make sense, since the definitions therein define the package as an application. Therefore your entire application should have a single node_modules at the root containing a single instance of Backbone.js.

@tbranyen I think you completely missed the point, and I'm sorry if I was not clear in anyway.

That sample application includes the sub_modules directory in that way, so I don't have to publish custom-collections and custom-models to the npm registry.

It is a perfectly valid use case for modules to have their own package.json files - in fact, that is how npm modules work. You write a module, define it's dependencies, and publish to the npm registry. Another module/application/whatever will then install your module via npm install module-name. Having Backbone.js not included in the module's package.json files is a stupid decision, as it means that the modules are then relying on an external dependency - very very bad practice, as modules include everything they need inside of it.

This should be re-opened.

@balupton

To put this this whole thing in bigger perspective. Query-Engine is a module, that provides a new QueryCollection which extends a Backbone.Collection. However, when you include Query-Engine in your app, and also include Backbone.js in your app, QueryEngine has it's own Backbone.js and so does your app, causing this situation to occur. Now to expect QueryEngine to not have a package.json is ludicrous! How can it be published?!?! Or, to expect QueryEngine to not include backbone.js as a dependency in it's package.json file is equally crazy. How on earth can we ensure QueryEngine will continue to work if the backbone.js dependency is external to QueryEngine. This completely breaks the point of using package.json to specify dependencies. In fact, using your logic, Backbone.js shouldn't even specify it's underscore dependency in it's package.json file, or even better yet not have a package.json file at all!

This is a serious issue, and if you feel that it isn't, then by all means post back asking for more clarification rather than just saying I'm doing it wrong and closing.

@balupton

There is nothing Backbone.js can do to solve this problem for you,

Yes there is, implementing the flag as suggested in the original post.

@balupton balupton added a commit to balupton/backbone that referenced this issue May 7, 2012
@balupton balupton Removed the harmful instanceof operator in preference for duck typing…
…. Fixes #1288
ac01492
@jashkenas
Owner

Here's where this is going wrong:

  • Creates a model instance using a custom model from Module A
  • Creates a collection instance using a custom collection from Module B
  • Adds the model instance to the collection instance

... you're mixing and matching two different versions of Backbone together. There's no guarantee at all that the models from version X will work together properly with the collections from version Y. If you don't mingle the two different versions, no error occurs. Right?

@balupton

If you don't mingle the two different versions, no error occurs. Right?

Correct, though there seems to be two different opinions here:

  1. You should never ever be using multiple backbone copies, as such anything to support it, be it whether or not it is compatible with existing code (passes tests) is irrelevant.

  2. Having backbone an internal module dependency, within different modules, is a perfectly valid use case. Support should be added for it, considering it is compatible with existing code (passes tests).

Personally, I do not understand why one would not pull the duck typing change, when it works, and enables a new use case. The only counter it seems is personal opinion of best practice, rather than expressed practical benefits.

Nor do I understand the practical basis of "never ever using multiple backbone copies", which seems to be again an argument just expressed as personal opinion. Finally, I do not understand why in this case, backbone is an exception to underscore - why under this opinion, are we allowed to include multiple underscore copies, but not multiple backbone copies? It seems the only reason is that the latter breaks due to the use of instanceof, which is something not willing to be fixed.

@jashkenas
Owner

You are allowed to use multiple Backbone copies, independently. You're mixing and matching models from one version with collections from another version, hence your problem.

It's like saying underscoreVersion2.each = underscoreVersion1.each, and then expecting the rest of underscoreVersion2 to continue to work.

@tomxtobin

I stumbled across this discussion while trying to figure out how instance testing gets handled with Node.js dependencies. I know I'm a bit late, but I think @balupton has been misunderstood.

Here's the issue, stripped down:

  • Package thing v1.0.0 gets submitted to the npm registry. It exports a Thing constructor.
  • Package thingutils v1.0.0 gets submitted to the npm registry. It requires thing v1.0.0, and has utility functions for creating Thing instances.
  • My app, foobar, requires thing v1.0.0 and thingutils v1.0.0. I install them both via npm, and therefore two separate package directories for the exact same version of thing, version 1.0.0, get installed:
    • One in foobar/node_modules/thing
    • One in foobar/node_modules/thingutils/node_modules/thing
  • I create a Thing instance in my foobar app using thingutils.
  • I now have no way of checking that this instance is an instance of Thing, even though my app and thingutils are both requiring the same exact version of the thing package, and thing.Thing is available to me. Node sees the two Thing constructors (and their prototypes) as different because they come from "different" modules with different filesystem paths.

I hope this helps someone else, as I was going a bit crazy trying to figure it out. This is the downside to the Node.js node_modules system: while you avoid versioning hell in resolving dependencies, you can't reliably test instances across modules the way you can in Python (where I come from) and other languages.

@balupton

Thanks for chiming in, turns out with:

  • One in foobar/node_modules/thing
  • One in foobar/node_modules/thingutils/node_modules/thing

If you do a rm -Rf node_modules; npm i inside foobar there will only be a foobar/node_modules/thing copy of thing, that thingutils will use.

You can further assist this if thingutils specifies thing inside peerDependencies rather than inside dependencies, you can also add thing inside devDepenencies for development etc. We have peerDependencies now, originally when this issue was posted, that wasn't a thing.

However, the unfortunate thing about this approach is it doesn't work with npm linking, as for some reason the requite won't follow up the chain properly (follows up the chain for the original location, not the location inside foobar). So you'll enounter this issue. Perhaps though this chaining up problem is actually a bug in node's module code... if so that would be awesome if it is fixed.

So far, keeping those things in mind I've been able to get around this issue with the need for duck typing... However, if this fixes it for all use cases not sure... at least for my use cases I've been able to get around it so far. Hope that helps.

@75lb

just lost hours to the "instanceof failing" issue within the node_module tree too, thankfully this cleared it:

(in the project folder)  
$ rm -rvf node_modules/
$ npm install
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment