search

miickel / gulp-angular-templatecache

Concatenates and registers AngularJS templates in the $templateCache.
MIT License
524 stars 103 forks source link

Don't use a logical or for setting header and footer. #147

Closed tandrewnichols closed 5 years ago

tandrewnichols commented 6 years ago

Regarding these lines: https://github.com/miickel/gulp-angular-templatecache/blob/master/index.js#L173-L174

Could you use something like 'templateHeader' in options ? options.templateHeader : TEMPLATE_HEADER for these? Because an empty string is falsey, which means I can't tell this module to use no header and footer and supply it myself later.

More specifically, this:

.pipe(ngtemplates({ header: '', footer: '' }))

still adds a header and footer.

simonua commented 6 years ago

@tandrewnichols, this is interesting. What is your use case where you would want to supply a header and footer later that may not be known at the time you initiate the template cache process?

tandrewnichols commented 6 years ago

I'm using gulp-remember-cache to only rebuild templates that change and then using gulp-header and gulp-footer to add the wrapper.

simonua commented 6 years ago

@tandrewnichols, would a not-so-elegant but functional work-around such as this suffice? I wouldn't be opposed to updating documentation to cover this - respectfully - edge-case.

.pipe(ngtemplates({ header: ' ', footer: ' ' })) instead of .pipe(ngtemplates({ header: '', footer: '' }))

tandrewnichols commented 6 years ago

Um, I guess. Without intending any offense, it seems like a bug, even if it is an edge case, only because it's slightly unexpected behavior. It seems like, if you pass something as the header and footer, those things should be used as the header and footer. The default should be reserved for the case where header and footer aren't supplied. Furthermore, it's a pretty small change right? It's arguably an even smaller (amount of code-wise) change than updating the readme.

All that being said, it's your library, so make what decision you feel is best.

simonua commented 6 years ago

@tandrewnichols, no offense taken. It's always good to have varying approaches and opinions on such. I just recently became a collaborator on this project as I worked to remove the gulp-util grab bag package, so I am combing through issues and PRs and seeing what's still applicable. That said, I don't have strong opinions about any particular approach, so long as it's a) not a breaking change, b) covered properly by unit tests, and c) intuitive and/or well-documented.

Would you be so willing to submit a PR for this request, please?

tandrewnichols commented 6 years ago

I could have a crack at it. Not sure how quickly I'll get to it, but I'll make a TODO so I don't forget.

simonua commented 6 years ago

That sounds awesome, Andrew! Looking forward to collaborating on it with you.

simonua commented 5 years ago

@tandrewnichols, thanks again for taking charge of this. Appreciate the collaboration.

Simon

tandrewnichols commented 5 years ago

Thanks for merging it so fast!