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

support dependencies declared with expressions #29

Closed
phillipgreenii opened this issue May 28, 2015 · 3 comments · Fixed by #31
Closed

support dependencies declared with expressions #29

phillipgreenii opened this issue May 28, 2015 · 3 comments · Fixed by #31

Comments

@phillipgreenii
Copy link
Contributor

In some usages of dojo, the modules are cleverly defined by specifying a string and splitting on it:

define('my module', 'dep1 dep2 dep3'.split(' '), function(dep1, dep2, dep3) {
  //i'm clever
});

This scenario isn't caught by deamdify, but it could be

@tbranyen
Copy link
Collaborator

Should it be? Dynamic construction of modules should not be supported IMO. I suppose it doesn't hurt if we can just migrate that section of code and retain functionality, but seems like a slippery slope.

For instance I would not want deamdify to fix:

var dynamicRequire = require;

require('../this/should/be/inlined');

for (var i = 0; i < someListOfModules; i++) {
  dynamicRequire(someListOfModules[i]);
}

@phillipgreenii
Copy link
Contributor Author

I agree, support for renaming require is beyond this project. I also agree that it is questionable to support things like 'dep1 dep2 dep3'.split(' '). However, my motiviation for using deamdify is to make it work with the Esri Javascript API and it uses that pattern. (this is how I discovered the problem)

@tbranyen
Copy link
Collaborator

Ah of course, damn you ESRI. I suppose handling that case may fix other poorly formed modules as well.

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 a pull request may close this issue.

2 participants