volojs / volo

Create front end projects from templates, add dependencies, and automate the resulting projects
https://volojs.github.io/
Other
1.41k stars 99 forks source link

Possible problem with "amd" #157

Closed uhop closed 11 years ago

uhop commented 11 years ago

I am not sure if it is an actual bug, or it just requires a little bit more explanations in the doc.

My understanding is that "amd" in package.json controls if -amd or -amdoff is applied to a package. No matter what I do, by default volo adds an unnecessary wrapper to every .js file. I have to instruct users to explicitly specify -amdoff when adding my packages.

Additional info: packages in question are heya/ctr and heya/pipe. They both use a version of UMD, and do not need a wrapper. The simplest case is heya/ctr because it doesn't have dependencies, yet exhibits the described behavior.

My commands:

volo create xxx
cd xxx
volo add heya/ctr heya-ctr

I tried adding and removing "amd" to its package.json as in the doc, which didn't affect anything as far as I can tell.

It works, if I add -amdoff:

volo add -amdoff heya/ctr heya-ctr
jrburke commented 11 years ago

The amd property in the package.json just indicates if the project wants AMD dependencies, and to trigger the scan for AMD usage. It is for project-level package.json files, and not for controlling how dependencies work. Even if it did, the heya/ctr code may still run into problems:

Looks like the heya/ctr code is using a UMD pattern that the AMD detection code cannot detect as an AMD call. Since the AMD scanning works via static analysis, it can only look for certain patterns. If there is a define(/*expected arg match*/) in the code, that works best, and the r.js optimizer uses a similar pattern to look for AMD usage.

So I suggest using one of the UMD patterns here: https://github.com/umdjs/umd

or switch to some UMD pattern where the define identifier is right next to the parens used around the define args, as that will work with any sort of "does this file use AMD" static scanning.

So going to close this as I believe the static issues will cause problems with other AMD scanning tools, and it is best to try a different UMD wrapper, but feel free to continue discussion in the ticket.

uhop commented 11 years ago

I will study the approved UMD patterns. Thank you for the suggestion and explanations.

While right now I have a workaround --- using -amdoff to install a package, but it relies on a bug that -amdoff is propagated to all dependencies of a package, not just the package itself. It would be helpful to have a flag in package.json somewhere in a volo section, which turns off any kind of code inspection/modification for a given package, but continued to apply regular behavior to dependencies. Obviously this is a low-priority enhancement request.

phated commented 11 years ago

@uhop dcl installs correctly with it's UMD, I think the problem exists in that put the entire UMD in a single line. If it is expanded, the RegExp should pick it up.