gulp-handlebars icon indicating copy to clipboard operation
gulp-handlebars copied to clipboard

Don't override other plugins file.defineModuleOptions

Open lfbittencourt opened this issue 10 years ago • 6 comments

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 avatar Aug 31 '15 02:08 lfbittencourt

@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 avatar Sep 01 '15 01:09 lazd

@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.

lazd avatar Sep 01 '15 01:09 lazd

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

lfbittencourt avatar Sep 01 '15 16:09 lfbittencourt

@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!

lfbittencourt avatar Sep 02 '15 00:09 lfbittencourt

Sounds like a plan, thanks for testing @lfbittencourt!

lazd avatar Sep 02 '15 18:09 lazd

@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 :-)).

lfbittencourt avatar Oct 19 '15 12:10 lfbittencourt