-
Notifications
You must be signed in to change notification settings - Fork 45
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
Templates compiled with Lodash aren't AMD compatible #18
Comments
Actually, on further review, it looks like this is already addressed in edge (if you're talking about precompiled templates). If you're not, then the |
I've added two pull requests (#20 & #21) to address this issue from the grunt-contrib-jst side. Both implement @eitanmk's suggestion:
Why two options?From a usage standpoint, they just provide a minor configuration syntax difference: // option 1
options: {
amdWrapper: true,
amdPathToUnderscore: 'override/path/if/you/want'
}
// option 2
options: {
amdWrapper: {
pathToUnderscore: 'override/path/if/you/want'
}
}
// or if you want to keep the default path (in both options)
options: {
amdWrapper: true
} The main reason there are two pulls is as a courtesy: I got on a roll and decided to refactor Option 1 does the job without that refactor. And it could be tweaked in favor of the other config syntax, of course. |
@colinhicks Why is this nessessary again? The issue was resolved in Lo-Dash (before this issue was even created). Shouldn't this issue be closed as |
@jdalton I came late to this conversation, so I may be misinterpreting context here, but my sense is that this was never a Lo-Dash issue, strictly speaking. The current _.template(...).source; Everything else – from processing settings to wrapping the output for AMD – is implemented by the grunt task itself. And that implementation is the source of this issue. It does not use the Lo-Dash cli to precompile templates, e.g. by spawning the equivalent of: $ lodash template="./*.jst"
# this would not manifest the grunt-contrib-jst issue |
@colinhicks Ah cool. Then why are you wanting to switch to Underscore? It doesn't at the time support AMD while Lo-Dash goes out of its way to support it, as well as ensuring that even its minified output can be used by various AMD optimizers. It seems like throwing the baby out with the bath water. Using Underscore will only defer the problem to when you have an |
Gotcha. None of this is predicated on a switch to underscore. This change just uses FWIW, here's my planned usage, with respect to my r.js config: paths: {
underscore: '../components/lodash/dist/lodash'
} Would this configuration change increase clarity, in addition to relevant documentation? options: {
amdWrapper: {
pathToUnderscoreOrLoDash: 'override/path/if/you/want'
}
} |
@colinhicks |
This library do the trick: https://github.com/terryweiss/grunt-template-module |
Not sure if this is really an issue for Lodash. With the switch of this module to use Lodash instead of underscore, I thought it should at least get mentioned here.
It seems that the way Lodash compiles templates is different from how underscore.js compiled them. Executing compiled Lodash templates in the browser relies on the presence of a global _ being defined, which AMD discourages.
Lodash compiled templates get the following in the template boilerplate after compilation:
I don't think underscore.js introduced such a dependency...
(In my setup, I'm actually wrapping frameworks for AMD and using noConflict to not leak any globals. So I won't have a global _ for this to use at runtime.)
Perhaps there should be a way to let the amdWrapper code know what AMD module name to insert as an AMD dependency (and use 'underscore' as a default)? So the line that gets inserted for the AMD would read like:
The text was updated successfully, but these errors were encountered: