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

Does 'namespace' work with almond? #77

Closed
vladikoff opened this issue Jan 10, 2014 · 6 comments
Closed

Does 'namespace' work with almond? #77

vladikoff opened this issue Jan 10, 2014 · 6 comments

Comments

@vladikoff
Copy link

Hey, I'm having issues loading a library created with almond using the dojo loader.

I have configured my library using this guide: https://github.com/jrburke/almond#exporting-a-public-api

When I try to load the library using Dojo's AMD loader it tries to fetch the files
i.e. from the guide return require('main'); it would try to load main and so on.

To prevent this I'm guessing namespacing require might help? However I was not able to get namespacing to work with almond. any suggestions? What changes should I make to the guide configuration ( https://github.com/jrburke/almond#exporting-a-public-api ) ?

@jrburke
Copy link
Member

jrburke commented Jan 10, 2014

We talked in IRC, and the issue was the dojo loader scanning and finding a require('dependency') call inside the UMD-wrapped factory. It is a bit odd it was matching on that, because it is technically not a CJS-style define() wrapping, but the fix was to just use requirejs('dependency') instead of require('dependency') to bypass its matching. Doing a var req = require; req('dependency') would also work.

namespace might also be an option, I just don't recall trying with almond specifically. It would have thought it would have worked, but if it did not, I would likely need to see the the output of the namespace build, to see what needs to be fixed.

But usually, wrapping the module in the UMD-style wrapper is enough, and is a bit cleaner than namespacing the calls. The closure around the code prevents the internals from affecting the outside code. The Dojo tool in this case seems to be overly eager in matching.

@jrburke jrburke closed this as completed Jan 10, 2014
@csnover
Copy link

csnover commented Jan 11, 2014

@jrburke I don’t understand. How is it odd that it was scanning and finding that require call? According to the AMD spec, “If the dependencies argument is omitted, the module loader MAY choose to scan the factory function for dependencies in the form of require statements (literally in the form of require("module-id")).” The dependency argument is omitted, so the factory function is scanned.

@jrburke
Copy link
Member

jrburke commented Jan 11, 2014

@csnover, ah I so I think I see the disconnect: I was thinking of the build scenario, where the factory function has to be a function literal passed to define to work (at least without more AST hoop jumping to find the definition of the variable passed in to the define() in this case). However, at runtime, Function.prototype.toString() will work, and Dojo does the right scanning there.

So @vladikoff, the more robust fix is to do what I first mentioned in IRC:

define([], factory);

The requirejs/require renaming works, but as @csnover out, an AMD loader will scan the function body at least during runtime. This way you do not need to rename, and it avoids a toString scan too.

@csnover
Copy link

csnover commented Jan 11, 2014

@jrburke I’ve run into this sort of thing more than once (componentjs/component#459) so I am curious to understand if there is something that your loaders are doing that enables people to incorrectly pass factories like this that needs to be corrected, or if there is something that needs to be updated in the spec to make sure that behaviour is consistent across all loaders.

@jrburke
Copy link
Member

jrburke commented Jan 11, 2014

@csnover it is probably worth specifying in the UMD patterns, to pass an empty array as first arg to define() if there are no dependencies. I'll work on an update to those patterns in the next couple of days.

@jrburke
Copy link
Member

jrburke commented Jan 12, 2014

Looks like the UMD patterns all mention dependencies, so I think they are OK. I have updated the README for this project, and also sent a pull request for the umd package used by some node modules here:
ForbesLindesay/umd#12

vladikoff added a commit to vladikoff/fxa-js-client that referenced this issue Jan 29, 2014
vladikoff added a commit to vladikoff/fxa-js-client that referenced this issue Jan 29, 2014
Update require definition. Based on requirejs/almond#77 (comment)

Update documentation, add style check to Travis

Add building to Travis as well.

Update Example
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

3 participants