Skip to content

add a new option, modulePrefix#10

Merged
phated merged 5 commits intogulp-community:masterfrom
mattma:master
Dec 19, 2014
Merged

add a new option, modulePrefix#10
phated merged 5 commits intogulp-community:masterfrom
mattma:master

Conversation

@mattma
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Comment thread test/test.js Outdated
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

phated commented Dec 19, 2014

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

@mattma
Copy link
Copy Markdown
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
Copy Markdown
Member

phated commented Dec 19, 2014

Awesome. Thanks. Will publish as 0.4.0 now

@phated
Copy link
Copy Markdown
Member

phated commented Dec 19, 2014

published.

@mattma
Copy link
Copy Markdown
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