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

Backbone.Relational does not work with require.js #215

Closed
wants to merge 1 commit into from
Closed

Backbone.Relational does not work with require.js #215

wants to merge 1 commit into from

Conversation

peters
Copy link

@peters peters commented Nov 18, 2012

Use factory pattern so that it also works with require.js

@glyphobet
Copy link

I can confirm that Backbone.Relational did work with require.js prior to commit 2de8020, which was made four days before this bug was opened.

Since that pull request was merged, require.js now raises this error when trying to load Backbone.Relational:

Uncaught Error: Module name "underscore" has not been loaded yet for context: _

Issue #57 appears to provide a solution very similar to the solution in this pull request, yet the discussion there seems to have drifted off topic.

Plus, this pull request says it has 1,670 additions and 1,697 deletions, which sounds suspicious to me, and GitHub won't display the diff. Perhaps something has converted between tabs and spaces, or converted newlines, or the like?

@glyphobet
Copy link

Yep, when you made this change your text editor or something also converted tabs into spaces across the entire file. I'm not affiliated with Backbone.Relational (other than being a user) but I personally wouldn't want to merge this into my project. :) I'd suggest you re-submit this pull request without the tab to space conversion.

@abrkn
Copy link
Contributor

abrkn commented Nov 20, 2012

How can we make this work with both browserify and requirejs?

@glyphobet
Copy link

I'm not familiar with browserify. Does the change in this pull request not work with it? Because, if I understand it correctly, the code here and in #57 is supposed to work with requirejs and with commonjs.

@PaulUithol
Copy link
Owner

Well, as I understand, these type of conflicts are the reason that Backbone and Underscore removed the factory pattern proposed here and in #57. In all probability, you can get any requirements manager individually to work fine using a small shim; trying to support them all inevitably seems to break support for yet another one.

Since relational depends on both Backbone and Underscore, I have a strong preference to keep relational's behavior and implementation consistent with those two. I'm assuming Backbone itself does function with both requirejs, commonjs, and browserify? Is there anything we can revert of remove to get relational back to that status?

@abrkn
Copy link
Contributor

abrkn commented Nov 20, 2012

Yes, Backbone.js (surprisingly) works with everything

@glyphobet
Copy link

Reverting 2de8020 would allow Backbone.Relational to work with requirejs again. But like I said, I'm not familiar with browserify so I can't speak to whether it's possible to make it work with browserify as well. How does Backbone solve this problem? If it works with everything, perhaps Backbone.Relational should copy how it does that.

@PaulUithol
Copy link
Owner

I'm not really at home with amdjs/curl/require/browserify et al., but does anyone know how (for example) Lo-dash (see the source at https://raw.github.com/bestiejs/lodash/v0.10.0/lodash.js) manages to support all of these, without the (IMHO) ugly closure?

@PaulUithol
Copy link
Owner

Or Backbone itself, for that matter.. it doesn't seem to do anything special at all indeed.

@hacking-robot
Copy link

does not work with require.js
Looking at the source shows that you made it to work only with CommonJS !!!

@abrkn
Copy link
Contributor

abrkn commented Nov 29, 2012

sigh, how can we make it work for both? we're all friends here

@PaulUithol
Copy link
Owner

Yep, I'd love to know that as well. Seems the whole package-management landscape is a proper jungle right now.

As a stop gap measure, I'm wondering if the following would allow both browserify and require.js to work (since they both supposedly use a global require function)?

// Support loading requirements using `require` compatible implementations. In server environments, `window` is not defined; in browser environments, check for the presence of `require`.
if ( typeof window === 'undefined' || typeof require !== 'undefined' ) {

I am wondering why typeof require !== 'undefined' doesn't seem to work for require.js though, when the function does exist, or it wouldn't be able to load anything on the next line.

@al
Copy link

al commented Nov 30, 2012

@PaulUithol I'm not sure where to comment there are so many options, but this thread contains the bulk of the discussion so here it goes.

4d6918b doesn't seem to cut it for Require.js at least.

I don't understand any of this stuff either, but as far as I can tell I want execution to enter the else clause (that's where control used to pass through pre- 2de8020), but because require is defined I end up in the if leading to "Uncaught ReferenceError: module is not defined".

I don't know how it effects people in the other camps, but as a Require.js user I'd be happy to see all this stuff removed and to be left to shim it myself. Supporting only one of them would be bad, and supporting all of them seems impossible ...

@philfreo
Copy link
Collaborator

I'm not really sure if there are any projects that do a good job supporting all of them so you should just look at other popular projects and do something similar. For example backbone-forms has a build script which produces an AMD version and a regular version

@michaelbrooks
Copy link

Yes, I am also getting the "module is not defined" error with require.js.

Replacing Line 21 with the following works for me:

        exports = Backbone;
        if (typeof module !== 'undefined') {
            module.exports = Backbone;
        }

Incidentally it would be cool if you added a 0.6.0 tag so I could nicely point my dependency management system at a more stable version of the library without worrying about these types of changes.

@hacking-robot
Copy link

I just forgot about backbone-relational as I spent two days trying to get
it right. And finally found it weird to use with the ids that must be in
the json first so the relations can be fetched.

2012/11/30 Alan Larkin [email protected]

@PaulUithol https://github.com/PaulUithol I'm not sure where to comment
there are so many options, but this thread contains the bulk of the
discussion so here it goes.

4d6918b 4d6918bdoesn't seem to cut it for Require.js at least.

I don't understand any of this stuff either, but as far as I can tell I
want execution to enter the elsehttps://github.com/PaulUithol/Backbone-relational/blob/master/backbone-relational.js#L24clause (that's where control used to pass through pre-
2de8020 2de8020),
but because require is defined I end up in the if leading to "Uncaught
ReferenceError: module is not defined"
.

I don't know how it effects people in the other camps, but as a Require.js
user I'd be happy to see all this stuff removed and to be left to shim it
myself. Supporting only one of them would be bad, and supporting all of
them seems impossible ...


Reply to this email directly or view it on GitHubhttps://github.com//pull/215#issuecomment-10893524.

@PaulUithol
Copy link
Owner

Hmm.. I added a 0.6.0 tag a while back.. but apparently, it hasn't gotten synced to github :/. I'll add that (and a 0.6.1 or 0.7.0`) as soon as we're back to a more stable version..

@michaelbrooks
Copy link

Just submitted a pull request (#220) with my solution to this problem, based on amdjs/backbone. I don't know if it works for all of the environments you are aiming to support, but it seems to work for require.js.

PaulUithol added a commit that referenced this pull request Dec 4, 2012
@PaulUithol
Copy link
Owner

@michaelbrooks : thanks, but I'm not really inclined to add that piece of code. It's not the most transparent pieces of code I know (to put it mildly), and it doesn't even work for all situations either. See the situation with underscore and backbone reverting it.

@michaelbrooks
Copy link

Yes, I didn't expect so. I hope you can figure out a workable solution.

@PaulUithol
Copy link
Owner

What Lo-dash (http://lodash.com/) does looks pretty clean to me, and they claim to be compatible with (at least) AMD. No idea how that goes with Browserify though.

Anyway, they got this at the top (in https://raw.github.com/bestiejs/lodash/v1.0.0-rc.2/lodash.js ):

  /** Detect free variable `exports` */
  var freeExports = typeof exports == 'object' && exports;

  /** Detect free variable `global` and use it as `window` */
  var freeGlobal = typeof global == 'object' && global;
  if (freeGlobal.global === freeGlobal) {
    window = freeGlobal;
  }

And this at the bottom of the file:

  /*--------------------------------------------------------------------------*/

  // expose Lo-Dash
  // some AMD build optimizers, like r.js, check for specific condition patterns like the following:
  if (typeof define == 'function' && typeof define.amd == 'object' && define.amd) {
    // Expose Lo-Dash to the global object even when an AMD loader is present in
    // case Lo-Dash was injected by a third-party script and not intended to be
    // loaded as a module. The global assignment can be reverted in the Lo-Dash
    // module via its `noConflict()` method.
    window._ = lodash;

    // define as an anonymous module so, through path mapping, it can be
    // referenced as the "underscore" module
    define(function() {
      return lodash;
    });
  }
  // check for `exports` after `define` in case a build optimizer adds an `exports` object
  else if (freeExports) {
    // in Node.js or RingoJS v0.8.0+
    if (typeof module == 'object' && module && module.exports == freeExports) {
      (module.exports = lodash)._ = lodash;
    }
    // in Narwhal or RingoJS v0.7.0-
    else {
      freeExports._ = lodash;
    }
  }
  else {
    // in a browser or Rhino
    window._ = lodash;
  }

Anyone have thoughts on this?

@abrkn
Copy link
Contributor

abrkn commented Dec 14, 2012

try it?

@jminstrell
Copy link

I use backbone-relational with the requirejs built in shim that I use with backbone and don't seem to have any problem so I'm not sure what the problem is here. But my version is old, 0.5.0. Has a newer version broken relational even with the shim? Over half my project's third party libraries need to be shimmed.

jQuery works with require.js and common.js if you need another example.

@philfreo
Copy link
Collaborator

philfreo commented Jan 9, 2013

I use Backbone and Backbone.Relational both as RequireJS shims. The current master of Backbone.Relational works fine as a shim. I'm assuming this pull request was to allow it to be used without a shim, which would be nice (if it worked properly), but not 100% necessary.

@PaulUithol
Copy link
Owner

Is the shim you guys are using a small, stand-alone file (I'm assuming it is?). If so, it might be a good idea to add it to the repo itself, so there's a defined and "generally accepted" solution?

@philfreo
Copy link
Collaborator

No a shim isn't a file. In RequireJS 2, a shim is just a line in the config file. See http://requirejs.org/docs/api.html#config-shim

My config.js looks (simplified) like this:

require.config({

    // Initialize the application with the main application file
    deps: ['main'],

    // 3rd party script alias names (leave off .js extension)
    paths: {
        'jquery': 'vendor/jquery-1.8.2',
        'underscore': 'vendor/underscore-1.4.2',
        'backbone': 'vendor/backbone/backbone',
        'backbone-forms': 'vendor/backbone-forms/distribution.amd/backbone-forms',
        'backbone-relational': 'vendor/backbone-relational/backbone-relational',
        'backbone-declarative': 'vendor/backbone.declarative/backbone.declarative',
        'backbone-super': 'vendor/backbone-super/backbone-super/backbone-super',
        'jquery-cookie': 'vendor/jquery-cookie/jquery.cookie',
    },

    // 3rd party scripts that are not AMD compatible
    shim: {
        'backbone': {
           deps: ['underscore', 'jquery'],
           exports: 'Backbone'  // attaches 'Backbone' to the window object
        },
        'underscore': {
            exports: '_'
        },

        // We have a special module 'backbone-all' (defined in 'backbone-all.js') that pulls in backbone
        // as well as all the Backbone.js plugins that we always use.
        'backbone-super': ['backbone'],
        'backbone-relational': ['backbone'],
        'backbone-declarative': ['backbone'],

        'jquery-cookie': ['jquery'],
    }

});

I do have a separate file, backbone-all.js, that's like this:

define([
    'jquery',
    'backbone',
    'backbone-super',
    'backbone-forms',
    'backbone-relational',
    'backbone-declarative',
],

function($, Backbone) {
    // Put any custom Backbone modifications here

    return Backbone;
});

and then I just require 'backbone-all' when I need Backbone.

Backbone-Forms for example has an AMD compatible distribution, so it doesn't require a shim. And Backbone itself wouldn't require a shim if I used the AMD version of it (https://github.com/amdjs/backbone).

That said, if Relational could become AMD compatible, that'd be better, as long as it was done properly.

@tomvo
Copy link

tomvo commented Mar 21, 2013

@philfreo Thanks for that snippet, helped us out in glueing all the pieces together. Any reason why not just also include underscore in the backbone-all module?

@philfreo
Copy link
Collaborator

Yes - it's not necessary to make underscore a dependency of backbone-all because it's already handled in the shim config for backbone itself (deps: ['underscore', 'jquery'],). The reason backbone-all exists is for Backbone plugins that depend on Backbone itself first being loaded. By the time backbone is there, underscore is already there too.

@brianjmiller
Copy link

Been fighting with this too. My issue is around the check for window and assuming that Backbone is on 'window' I think. I get:

"Uncaught TypeError: Cannot set property 'Relational' of undefined"

Even with the shim suggested by @philfreo. I'm using Almond.js to build essentially the same as: https://github.com/phawk/Backbone-Stack I gather that even in a browser environment in that case Backbone should be getting found on something called exports, or the like. So the check for exports ought to be happening before trying to find Backbone on window.

Happens at:

var _, Backbone, exports;
if ( typeof window === 'undefined' ) {
    _ = require( 'underscore' );
    Backbone = require( 'backbone' );
    exports = module.exports = Backbone;
}
else {
    _ = window._;
    Backbone = window.Backbone;
    exports = window;
}

    // error here, Backbone is not defined
Backbone.Relational = {
    showWarnings: true
};

@brianjmiller
Copy link

Figures 10 minutes after posting comment I find an okay workaround here: requirejs/r.js#197

In case anyone else runs into this (or it helps -relational in some way) I switched to not use the AMD version of Backbone and it appears to be working.

@PaulUithol PaulUithol mentioned this pull request Jul 9, 2013
@philfreo philfreo mentioned this pull request Nov 21, 2013
@fredgate
Copy link

Backbone relational does not work well with requirejs and amd version of backbone.
This test is not exact :

if (typeof window === 'undefined') {
  ...
}
else {
  _ = window._;
  Backbone = window.Backbone;
  exports = window;
}

because in a browser the window object is well defined, but backbone is not declared as a global variable as it is the amd version that is used.

Why not use the factory pattern to support module. It is possible to take inspiration from backbone.validation that does this very well.

@quentindemetz
Copy link

+1
I'm having issues using Backbone 1.1.1 which has AMD support.

@brianjmiller
Copy link

+1, here I am back on this issue. Backbone 1.0.0 works Backbone 1.1.2 doesn't.

@PaulUithol
Copy link
Owner

Do you mean you can't get it to work at all anymore, not using a stub either?

Either way, sounds like about the right time to patch backbone-relational for AMD, now that Backbone supports it out of the box as well.

@PaulUithol
Copy link
Owner

Okay, having a go at this... going to need a little help in testing this on some real-life applications later on. Would the following work? Specifically, I'm assuming here that exports is the correct var to use for named scopes; is it?

( function( root, factory ) {
    // Set up Backbone-relational for the environment. Start with AMD.
    if ( typeof define === 'function' && define.amd ) {
        define( [ 'exports', 'backbone', 'underscore' ], factory );
    }
    // Next for Node.js or CommonJS.
    else if ( typeof exports !== 'undefined' ) {
        factory( exports, require( 'backbone' ), require( 'underscore' ) );
    }
    // Finally, as a browser global. Use `root` here as it references `window`.
    else {
        factory( root, root.Backbone, root._ );
    }
} ( this, function( exports, Backbone, _ ) {

@PaulUithol
Copy link
Owner

Also, do we need to return anything from the factory function? Answers to that seem to contradict each other..

@brianjmiller
Copy link

Yes, I was unable to get it to work with a shim and without, I also took a stab at an init function but it might have just been too late at night for my brain to get that one going (it never got run FWIW). The code snippet above looks closer to what I've seen of late, I've had good luck in another project (sorry it isn't open (yet) or I'd link it here) using the UMD approach. You might look to it for an answer about the return part. See https://github.com/umdjs/umd, in particular https://github.com/umdjs/umd/blob/master/returnExports.js. It will probably be tonight (for me) before I can take another stab at it.

@PaulUithol
Copy link
Owner

Could you check if master works for you? See 3a3acee. I did look at the UMD stuff, but it's not clear to me what we'd need the return for; everything we do goes under the Backbone.* umbrella.

@brianjmiller
Copy link

Master appears good to go. Works in my setup and I was able to remove the shim completely, so 👍. Yes, probably nothing to be returned.

@PaulUithol
Copy link
Owner

Alright, closing this one. Will tag it as a release soon.

@PaulUithol PaulUithol closed this Mar 3, 2014
@dminkovsky
Copy link

+1. Thank you for merging this, and incredible thanks for Backbone-relational in general.

The "problem" is that Backbone just went AMD. So, if you try to use the one-file optimizer with this latest AMD version of Backbone and non-AMD Backbone-relational, Backbone-relational is evaluated before Backbone, even with proper configuration and dependency configuration. It's a synchronicity issue related to how define() works, and makes sense if you step through it.. You can read more at http://stackoverflow.com/a/12504784/741970.

Has anyone tried this with Almond?

@brianjmiller
Copy link

@dminkovsky I'm able to use BB 1.1.2, almond 0.2.9, and BB.relational git master together without issue using grunt-contrib-requirejs.

@dminkovsky
Copy link

Hi @brianjmiller, thank you! Was wondering, for a minute, if anyone tried the non-master (non AMD) version with almond, because maybe almond uses define() synchronously.

Anyway... I've since just rolled over to Backbone relational master too, and as you can see above, am trying to spread the gospel.

@brianjmiller
Copy link

Yes, I had, which is what originally led me back to this issue.

@dowglaz
Copy link

dowglaz commented Dec 10, 2015

Hello guys.

I know this discussion is old, but I faced some terrible hours trying to figure out why including this library on my project was causing it to completely break...

I'm using Backbone and requirejs on my project and I was making the shim workaround and nothing was working. So I looked into the source code of backbone-relational and noticed that there's two explicit requires of 'backbone' and 'underscore' on line 51. The problem is that I'm rather using my libraries in 'lib/backbone' and 'lib/underscore' instead of what's (implicitly) specified there. Even declaring the paths: on requirejs.config, didn't work.

In the end I found a configuration that just works that is:

requirejs.config({
  //...
  paths: {
    'underscore': 'lib/underscore',
    'backbone': 'lib/backbone'
  },
  shim: {
    'lib/underscore': {
      deps: ['lib/jquery'],
      exports: '_'
    },
    'lib/backbone': {
      deps: ['lib/underscore', 'lib/jquery'],
      exports: 'Backbone'
    },
    'backbone-relational': ['backbone'],
  }
});

And also force requiring it without the "lib" prefix (because require 'lib/backbone' and 'lib/underscore' doesnt' work):

define([
    'lib/jquery',
    'backbone',
    'underscore',
    'backbone-relational'
  ],
  function ($, Backbone, _) {
    return Backbone;
  }
);

In other words, I was forced to change somethings on my project (worst, I had to found this practically at random, since I was not understanding what was happening .. I know it's my fault also, but I'm kinda newbie on JS and requirejs :P) and I noticed that there's a possible solution, at least for requirejs: just remove the lines 50-52 may work since the programmer make a shim exporting 'backbone' to window.Backbone and 'underscore' to window._ .

It's less obtrusive and don't force the programmer to make a lot of changes on the project just because of one dependency. And it just works, regardless of whether there's another export of 'backbone' to any other variable, nothing blocks you from making more than one shim.

@philfreo
Copy link
Collaborator

@dowglaz I think you're over complicating things. You shouldn't need shims / you shouldn't need to define the dependencies of backbone explicitly if you're using the AMD versions of everything which work well together.

My require js config is pretty simple:

paths: {
    'underscore': 'vendor/underscore',
    'backbone': 'vendor/backbone/backbone',
    'backbone-relational': 'vendor/backbone-relational/backbone-relational',
}

And I have one additional file/module called 'backbone-all' that I use & require anywhere that I expect to load backbone with all my backbone plugins:

define([
    'underscore',
    'backbone',
    'backbone-relational',
],

function(Backbone) {
    // Put any custom Backbone behavior here

    return Backbone;
});

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

Successfully merging this pull request may close these issues.