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

Built File Doesn't Work w/ AMD #68

Closed
matthewwithanm opened this issue May 29, 2013 · 18 comments
Closed

Built File Doesn't Work w/ AMD #68

matthewwithanm opened this issue May 29, 2013 · 18 comments

Comments

@matthewwithanm
Copy link

The packaged versions of the script don't work with AMD loaders because, even though they define the dependencies ('eventEmitter' and 'eventie'), they aren't registered with a name.

  define( [
      'eventEmitter',
      'eventie'
    ],
    defineImagesLoaded );

So when this point is reached, the loader doesn't know where to find either of them.

@desandro
Copy link
Owner

Use Bower

bower install imagesloaded

This installs imagesLoaded and dependencies in another directory (currently components/).

You can then set up paths for the dependencies in your Require.js settings (I'm assuming you're using Require.js. Otherwise, I'm not sure how this works in AMD).

require.config({
  paths: {
    'eventie': 'components/eventie/eventie',
    'eventEmitter': 'components/eventEmitter/EventEmitter',
    'imagesloaded': 'components/imagesloaded/imagesloaded'
  }
});

I would also take a look at RequireJS + Bower Grunt task

AMD isn't quite my thing. So if there's something here that doesn't make sense, please chime in and help me out.

@matthewwithanm
Copy link
Author

Thanks! I know about Bower but I'm using volo. Like with Bower, you can install the dependencies as separate scripts and then configure requirejs so it knows where to find them (which is what I'm doing to get it work).

But this issue is really more about the built version of the script: not only can't it be loaded with an AMD loader (requirejs) but it also can't be used on a page that has one (even if it's not used to load it).

In order to fix the issue, I think the grunt task is going to have to do more than just concat the scripts. Basically, the define calls for EventEmitter and eventie need to passed the module id. This is done automatically when you optimize the script with r.js (which is the way I've handled this in the past).

@desandro
Copy link
Owner

I see. My idea was that the packaged version was for users who are not using AMD. They just want the script, wham-bam-thankya-ma'am.

So maybe the more pertinent problem is that imagesLoaded does not currently support volo. If dependencies were available in volo, with this resolve the matter?

@matthewwithanm
Copy link
Author

Volo doesn't have its own registry; it just uses GitHub. Adding a package.json with volo dependencies would be cool too, but my issue was really about the built file not working with AMD. In other words, giving AMD users—who aren't necessarily using a package manager at all—the "just the script" option.

I opened a pull request to address it. It produces virtually identical code with the only difference being that the modules get named.

@desandro
Copy link
Owner

I appreciate the contribution, but I feel the whole point of AMD is that it loads Modules. So either use Bower, or manually manage the dependencies. The packaged source file is designed for quick-and-straight forward in-browser use.

@thisispete
Copy link

I'm having trouble getting this to work with Require JS as well. I'm not totally understanding the whole thread here, just wanted to vote up the issue.

@sjonnet19
Copy link

I had the same problems needed to remove the AMD closure

@matthewwithanm
Copy link
Author

You shouldn't need to modify the source. If you're using it with AMD (but without Bower), you should be find if you do three things:

  1. Use the "un-packaged" JS.
  2. Add the dependencies (@Wolfy87's eventEmitter and @desandro's eventie) to your project.
  3. Update your requirejs paths configuration so it can find those modules (if necessary)

My issue was about using the packaged version with AMD/require, but that wasn't in the cards.

@matthewwithanm
Copy link
Author

@desandro It might be nice for the README to mention the dependencies. As it stands, it seems like the non-Bower way to install is to use the packaged version, even though that isn't really the case for AMD users.

desandro added a commit that referenced this issue Jun 11, 2013
@desandro
Copy link
Owner

Okay. done.

@matthewwithanm
Copy link
Author

👍

@desandro
Copy link
Owner

Let me do a proper follow-up.

My aim is to support both RequireJS and straight-up <script src="easy-file-include.js"></script>. imagesloaded.pkdg.js satisfies easy-file-include.js.

For RequireJS, things are a bit tricky. On a user's machine, there's no way to guarantee the path to the dependencies. I could guess that they're in components/component_name/component_name if you're using Bower, but even that path is configurable.

The solution I have chosen, is to require eventie and eventEmitter, and ask that these paths be provided by the user in their Require JS config. This isn't ideal.

If there's a better, more user-friendly way to support both RequireJS and easy-file-include.js, I'm happy to hear your feedback.

@matthewwithanm
Copy link
Author

That seems fine to me. AMD modules aren't tied to a specific package manager so the installation instructions can't be much more specific. It wouldn't hurt to add support for more package managers (I think I mentioned how volo uses package.json), but you might end up adding a bunch of junk for a bunch of different JS package managers. Bottom line is that you're using AMD and telling people where to get the dependencies.

Of course, I still think it makes sense to build the packaged version with the r.js optimizer (:

@Olical
Copy link

Olical commented Jun 11, 2013

If you are going to build a fully packaged version: I'd recommend using r.js, as @matthewwithanm mentioned, as well as Almond. It's a lighter version of RequireJS that you can bundle with an r.js optimised version of your script.

So you can still use AMD but produce a built script that has no external dependencies at all. I personally think you should only provide the unminified AMD source and let the users decide how they want to build and package it with their code. But it seems like that puts a lot of users off.

@jriesen
Copy link

jriesen commented Jul 9, 2013

Although the readme says "Install imagesLoaded and its dependencies" and provides the config... the only actual links to those dependencies are here in this Issue, not in the README. (Note: I'm using RequireJS but not Bower.) I had to Google for "imagesloaded eventemitter eventie" to get here.

EDIT: I am now using Bower, so it may be a moot point, but it was sorta annoying at first trying to get the dependencies in order when I was just downloading scripts manually from github.

@desandro
Copy link
Owner

imagesloaded.pkgd.js is now generated with RequireJS, so you can require imagesloaded.pkgd.js. See http://desandro.github.io/imagesloaded/#requirejs

Thx @matthewwithanm for your insight here!

@nitinsurana
Copy link

I have the below in require js path :
'imagesloaded': 'vendor/imagesloaded.pkgd',

But when I try to use r.js to optimize, it gives
No such file or directory eventie/eventie.js

Can you please let me know how to solve ?
Also, the current readme is different (doesn't contain anything regarding eventie) than the commit link specified in one of the above comments.

@desandro
Copy link
Owner

@nitinsurana See docs, you do not need to set a path for imagesloaded if you are using the pkgd file. http://imagesloaded.desandro.com/#requirejs

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

No branches or pull requests

7 participants