Skip to content
This repository has been archived by the owner on Mar 26, 2018. It is now read-only.

Shimming Backbone and Underscore for RequireJS unnecessary? #101

Closed
raphaelvalerio opened this issue Jul 17, 2013 · 17 comments · Fixed by #238
Closed

Shimming Backbone and Underscore for RequireJS unnecessary? #101

raphaelvalerio opened this issue Jul 17, 2013 · 17 comments · Fixed by #238

Comments

@raphaelvalerio
Copy link
Contributor

Hi everyone,

When you scaffold an app with generator-backbone and choose to use RequireJS, the resulting main.js file shims both Backbone and Underscore in require.config.

However, the Bower installs for both of these libraries are the AMD versions, and as far as I can tell, they both are fully AMD compatible out of the box. According to the RequireJS documentation, you should never shim a module that already has a define() call as it can cause trouble.

On my test app, I've commented out the shim declarations and have no trouble with using Backbone or Underscore in my RequireJS-rigged app, so as far as I can tell the shims are unnecessary.

Am I missing something here? Is there a reason that you're shimming Underscore and Backbone that I'm too stupid to be aware of, or should the shim stuff be removed?

@revathskumar
Copy link
Member

@raphaelvalerio I am not a requirejs expert. but surely get someone to comment on this and fix this.

@addyosmani / @passy Please help to resolve this.

@raphaelvalerio
Copy link
Contributor Author

@revathskumar @addyosmani @passy If the shims should indeed be removed, I can easily submit a pull request with the fix. Just confirm that they shouldn't be there, and I'll take care of it.

@jridgewell
Copy link

I think it's better to leave the shims in place, and instead use the actual Backbone and underscore.

@rocky-jaiswal
Copy link

I think if the generator makes it explicit (in the readme or the description) that the downloaded versions of Backbone and Underscore are the AMD compatible versions there is no need of the shims.

Conversely, we can use the vanilla versions and use the shims only.

My only concern is that if a new version comes out and somebody upgrades with a non-AMD version things can break if they are no aware of our choice.

@revathskumar
Copy link
Member

So I think we can keep the shim and use the non-AMD (official) backbone and underscore. If so we need to change only the bower.json.
Can anyone confirm that latest backbone and underscore works fine with shims.

@jridgewell
Copy link

I've been using the non-AMD ones, and everything is working the same.

@raphaelvalerio
Copy link
Contributor Author

Remember that the generator offers you a choice on the command line during scaffolding, where you can choose whether you want to install RequireJS for AMD support or not. This choice totally changes the way the scaffolded app ends up being created.

If you DO choose to install RequireJS, here's what happens:

  • Bower.json is generated with underscore-amd and backbone-amd in its dependency lists (note the '-amd'). This is a fork of the regular Backbone and Underscore packages. They're totally separate packages.
  • It also (obviously) puts requirejs in the Bower.json dependencies, and creates app/scripts/main.js with a require.config and require() declaration that fires up RequireJS and defines the path to Backbone in the bower-components/backbone-amd path, in other words, the AMD-compatible forked version of Backbone

If you DO NOT choose to install RequireJS, here's what happens:

  • Bower.json is generated with underscore and backbone as its dependencies. These are the regular non-AMD versions of Backbone and Underscore, without AMD support.
  • requirejs is not installed, and the app/scripts/main.js file has, of course, no require.config or require() code. No shimming here.

The point of all of this is that if you scaffold an app with yo backbone and you choose to include RequireJS in the command-line interface, RequireJS is installed with the AMD-compatible forked versions of Backbone and Underscore, backbone-amd and underscore-amd. These versions should not need to be shimmed because, unlike the vanilla versions of the libraries, they are already AMD-ready. If you don't install RequireJS, you get vanilla versions of both libraries, no shimming, nothing.

You should not need to shim an AMD-ready JS file, and according to the RequireJS documentation, doing so can cause the potential for trouble. I've removed the shim declarations from the require.config call in app/scripts/main.js on my test bed and there hasn't been any trouble. Since these are the AMD versions, I don't see why they should be shimmed at all (unless the Yeoman team have a particular reason that I'm not aware of for doing so).

Think of the choice made at scaffold as implicitly creating a forked version of generator-backbone, one without RequireJS (generator-backbone) and one with (generator-backbone-amd). We don't have to worry about people running into problems updating Underscore and Backbone in their apps because Bower is installing two different forks of Underscore and Backbone depending on whether this is the AMD or non-AMD version of the scaffolded generator.

@rocky-jaiswal
Copy link

That is right. I just ran the generator and it does ask if I want Require.js support and then downloads the AMD version. I also tested it without the shims (I only needed to give the paths for Backbone and Underscore) and everything worked.

@jridgewell
Copy link

Let me clarify: I am running the Requirejs scaffolded install with the official libraries (and the shims they need) and everything is working fine.

The shimming system itself is robust, and as long as the libraries keep everything they need under a single object namespace, it's not going to break.

I think it's better if we think of the scaffolding as generator-backbone and generator-backbone-requirejs, using the official libraries, and shimming to get Requirejs working.

@raphaelvalerio
Copy link
Contributor Author

@jridgewell I'm not sure I understand what you think we should be doing (although I do understand and definitely agree with what you mean about making a distinction between the version scaffolded with and without RequireJS as if they were almost separate generators). Do you think we should be leaving the shims in place in the Require version of the scaffolded app or do you think they should be removed?

I'm guessing you're saying that we should leave them in, so I'll respond under that assumption (please correct me if I'm wrong) --

If we scaffold with RequireJS, the shims for Underscore and Backbone aren't necessary because the versions of Underscore and Backbone are AMD forks of those libraries and don't need to be shimmed. They work without the shims.

They also seem to work with the shims, but here's the quote from the RequireJS docs that suggests that's not a good idea:

Remember: only use shim config for non-AMD scripts,
scripts that do not already call define(). The shim
config will not work correctly if used on AMD scripts,
in particular, the exports and init config will not
be triggered, and the deps config will be confusing
for those cases.

Both underscore-amd and backbone-amd most definitely have define() calls in them, so this quote would seem to apply to them.

So from my point of view, there are two big reasons to remove the shims from the RequireJS version of this generator: 1) Because the RequireJS docs say shimming an AMD module is a bad idea, and 2) it's unnecessary and just wastes bytes.

Unless there's a genuine reason why those shims must be there that I'm unaware of, I contend that we should remove them from the RequireJS version of main.js.

@jridgewell
Copy link

You're correct, I think the shims should be left in, but only because we should switch away from backbone-amd and underscore-amd and instead use the non-AMD official libraries. I have more trust in Requirejs' shimming being able to work with the official code then I do in the *-amd forks being current with new releases.

@raphaelvalerio
Copy link
Contributor Author

@jridgewell Ah, now that's an interesting point. I kind of like that idea. I don't know anything about the people behind the -amd forks, but I would usually prefer to rely on the official projects too, just on principle.

To distill down this shims issue (unless the main authors of backbone-generator have a clear reason why they're shimming the AMD versions of these libraries, which seems unlikely), there seems to be two different directions to take.

When the user chooses to install RequireJS during scaffolding, either:

  1. Replace the -amd forks of the Backbone and Underscore libraries with the core (non-amd) versions, which are then shimmed in main.js
  2. Keep the -amd forks of Backbone and Underscore, and remove the shim directive from require.config in main.js

I'm leaning toward option 1, like @jridgewell, because I do like the idea of sticking to the main Backbone and Underscore release paths.

What do others think?

@fiznool
Copy link

fiznool commented Jul 23, 2013

Number 1. is the way to go. FWIW I've been using Backbone + Underscore non-AMD with the shim config for a while and there are no issues.

@revathskumar
Copy link
Member

Thanks everyone for the comments.
We will go for option 1 Replace the -amd forks of the Backbone and Underscore

@raphaelvalerio Can you please help to fix this with a PR?

Thanks.

@raphaelvalerio
Copy link
Contributor Author

Done! I'll close this issue thread so we can move the discussion over to the pull request if needed.

revathskumar added a commit that referenced this issue Jul 23, 2013
Fixes #101 : Use (and shim) core Backbone and Underscore Bower packages for scaffolding with RequireJS
@lukaszprus
Copy link
Contributor

Hi All,

I just wanted to ask if this needs updating as starting from version 1.1.1

"Backbone now registers itself for AMD (Require.js), Bower and Component, as well as being a CommonJS module and a regular (Java)Script. Whew."

See http://backbonejs.org/#changelog.

Same for Underscore starting from 1.6.0

"Underscore now registers itself for AMD (Require.js), Bower and Component, as well as being a CommonJS module and a regular (Java)Script. An ugliness, but perhaps a necessary one."

See http://underscorejs.org/#changelog.

So no need for shims?

@passy passy reopened this Mar 31, 2014
@passy
Copy link
Member

passy commented Mar 31, 2014

Exciting! Let's investigate that. I hate that shimming nonsense.

passy added a commit that referenced this issue Apr 3, 2014
Fixes #101

Backbone and Underscore are now properly AMDified, so we don't need to define a
shim config for them anymore.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants