Allow double quotes in template strings #2121
Conversation
| @@ -107,6 +107,7 @@ L.Util = { | |||
|
|
|||
| compileTemplate: function (str, data) { | |||
| // based on https://gist.github.com/padolsey/6008842 | |||
| str = str.replace(/"/g, '\\\"'); | |||
There was a problem hiding this comment.
This should be chained with the next line, right? And if this is going to be a continuing oddness I really wonder if fancy template compilation is worth it.
|
Not sure we want to make this a general-purpose templating utility... I made it lightweight specifically for templating URLs, and it's unlikely to see doubles quotes in URLs... |
|
Even though, it seems like if the thesis is "interpolate variables in a string" then the behavior shouldn't have an unpredictable implementation-specific quirk like this. |
|
Then we should probably also fix line breaks, all kinds of weird symbols, etc. Or we could just updated the docs to state that it's specifically used to interpolate URLs. |
|
Yeah... part of my unease here is because the old-fashioned non-compiled version was less quirky and the change didn't seem justified by perf in #1968 |
|
After some clean up, it turned out to be quite short so I think it was justified :) |
|
I think this is a case in which short != simple, especially in terms of behavior. |
|
Maybe, but I like it. Leaflet was never meant to have powerful general-purpose framework facilities after all, and it works great for Leaflet needs. |
|
'Simble templating facilty' doesn't sound to me like something that does not support something common like double quotes. You might consider the ease of writing and reading the docs for it as well. |
| <script> | ||
| mocha.setup({ | ||
| ui: 'bdd', | ||
| ignoreLeaks: true |
There was a problem hiding this comment.
Could you also comment on why this is needed?
There was a problem hiding this comment.
mocha.ignoreLeaks() did not make mocha ignore leaks at my setup, resulting in lots of tests failing, while this syntax fixes that.
mocha.ignoreLeaks(true) also does the job. This method was changed in Mocha (mochajs/mocha@d6330cb)
I should have done it in a separate pull though, shall I make a new PR for the (non-template) spec related stuff and remove it from this one?
There was a problem hiding this comment.
Interesting, I wonder what's different in your setup if Travis builds current Leaflet just fine...
There was a problem hiding this comment.
Was asking myself that question too...
If I put back the original
mocha.setup('bdd');
mocha.ignoreLeaks();
it fails again, if I add true, the leaks are properly ignored...
$ npm list | grep mocha
├── karma-mocha@0.1.0
├─┬ mocha@1.13.0
Allow double quotes in template strings
|
Decided to merge. Thanks @jieter! |
Fixes #2120.
And: