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

Don't override other plugins file.defineModuleOptions #60

Open
lfbittencourt opened this issue Aug 31, 2015 · 6 comments
Open

Don't override other plugins file.defineModuleOptions #60

lfbittencourt opened this issue Aug 31, 2015 · 6 comments

Comments

@lfbittencourt
Copy link

I will try to be short and describe an issue I've faced here...

I precompile my Handlebars templates as AMD modules. In this templates, I have calls to custom helpers I've created. As I keep these helpers as AMD modules too, it would be nice parse that templates, find any custom helper call and inject them as dependencies to the precompiled module. Phew!

Why can't I do that? Because any file.defineModuleOptions.require I have is overwritten in https://github.com/lazd/gulp-handlebars/blob/master/index.js#L52.

Here you can see gulp-define-module author showing a way we can be sure we don't override other plugins. Does it make sense for you?

Thanks!

@lfbittencourt lfbittencourt changed the title Don't override other plugins file.defineModuleOptions Don't override other plugins file.defineModuleOptions Aug 31, 2015
@lazd
Copy link
Owner

lazd commented Sep 1, 2015

@lfbittencourt Thanks for the clear and thorough issue report. I totally understand what you're saying, I'll get a fix in for this soon.

@lazd
Copy link
Owner

lazd commented Sep 1, 2015

@lfbittencourt can you try my fix for this issue by running these commands and report back?

npm remove gulp-handlebars
npm install git://github.com/lazd/gulp-handlebars.git#issue/60

Then, try your build and see if the module definition happens as expected. I wasn't sure what to do with the context and wrapper properties, but I made sure I didn't override any object on file.defineModuleOptions.

@lfbittencourt
Copy link
Author

Thanks! I'll come back here as soon as I have tested it :-)

@lfbittencourt
Copy link
Author

@lazd It's working smoothly now! About context and wrapper, I'd do exactly what you did.

Looking forward to the merge. A major version to fix #61 as well, maybe?

Thanks again for the quick fix!

@lazd
Copy link
Owner

lazd commented Sep 2, 2015

Sounds like a plan, thanks for testing @lfbittencourt!

@lfbittencourt
Copy link
Author

@lazd I see there's some discussion about #61, so this issue could be released as a minor version. There's no relation between the issues and it would be very nice release the fix ASAP (at least to me :-)).

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

2 participants