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

add a new option, modulePrefix #10

Merged
merged 5 commits into from
Dec 19, 2014
Merged

add a new option, modulePrefix #10

merged 5 commits into from
Dec 19, 2014

Conversation

mattma
Copy link
Contributor

@mattma mattma commented Dec 18, 2014

I am working on Ember-Cli enabled Ember application. It requires a prefix before each AMD module. For example, If I have a file path of client/app/templates/application.hbs and I run gulp-wrap-amd with options.moduleRoot: 'client/app/', I have an output amd module name of define("templates/application",. It is good. But I want to add an prefix to it. So the output I want is define("rocks/templates/application", with modulePrefix: 'rocks'.

With my fork, it detect moduleRoot options, only execute it when it is defined. then, it will check user's input value on moduleRoot, prepend modulePrefix plus / to options.name. So we get an end result of define("rocks/templates/application"

When I uses gulp-define-module with the customization of here. It requires a lot of customization work. Then I fork the gulp-define-module project to simply things. Then I saw your comments. I have a working solution for my case.

    .pipe($.wrapAmd({
      deps: ['exports'],          // dependency array
      params: ['__exports__'],        // params for callback
      moduleRoot: 'client/app/', // include a module name in the define() call, relative to moduleRoot
      modulePrefix: 'rocks/'
    }))

I have fully tested in my local environment. It works great. If you omit the modulePrefix option, it will do the exactly same thing like what you currently have. So it is completely backward compatible update.

If you think it benefits for the project, I would like to update documentation, tests, along with the fork. Thanks. @phated

@phated
Copy link
Member

phated commented Dec 18, 2014

This seems to match nicely with the moduleRoot config option (I had actually forgotten that support was added for it). If you add docs and tests for this I think we could get it merged.

P.S. - I like that you didn't need to modify the template.

@mattma
Copy link
Contributor Author

mattma commented Dec 18, 2014

Thanks. I will implement those soon. P.S. - I like that you didn't need to modify the template. --- Do one thing only in gulp plugin. I must follow the rule. :)

@mattma
Copy link
Contributor Author

mattma commented Dec 18, 2014

@phated TAP testing framework is an very interesting choice. Is this better than Mocha? Is it a generic testing framework or is it only for stream testing? I have been using Mocha, Should for a long time.

@mattma
Copy link
Contributor Author

mattma commented Dec 18, 2014

@phated I have updated the ember-htmlbars example on my gulp-handlebars PR which should be merged anytime soon. And if you are curious on how I am solving the problem of modify the template. Here is my solution

.pipe(task({
modulePrefix: 'rocks/',
deps: ['jade'],
params: ['jade'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing comma

@phated
Copy link
Member

phated commented Dec 18, 2014

Just a few comments. Otherwise, it looks good. I chose to use TAP just because it feels a lot less heavy than mocha. I have been liking lab recently, but I'm not going to go back and rewrite all the tests.

@phated
Copy link
Member

phated commented Dec 19, 2014

Looks like 2 trailing commas were missed in the cleanup. The rest looks good.

@mattma
Copy link
Contributor Author

mattma commented Dec 19, 2014

@phated Removed all the trailing commas.

phated added a commit that referenced this pull request Dec 19, 2014
add a new option, modulePrefix
@phated phated merged commit 53fcb6e into gulp-community:master Dec 19, 2014
@phated
Copy link
Member

phated commented Dec 19, 2014

Awesome. Thanks. Will publish as 0.4.0 now

@phated
Copy link
Member

phated commented Dec 19, 2014

published.

@mattma
Copy link
Contributor Author

mattma commented Dec 19, 2014

Cheers! :)

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 this pull request may close these issues.

2 participants