For the jsRender.templates() method (in NodeJs) paths without './' as prefix are not recognized

[jandrieu]

Unfortunately, both absolute paths and relative paths without the initial './' fail to be recognized as paths when calling jsRender.templates(path);

Given a current working directory of /home/joe/website/templates/ and three files

• local.html
• mainTemplate.html
• relative.html

with relative.html containing the following (the contents of the other two files don't matter):

{{:key}}

run the following script:

#!/usr/bin/env node
jsRender = require('jsrender');

var tmpl1 = jsRender.templates('local.html');
var tmpl2 = jsRender.templates('/home/joe/website/templates/mainTemplate.html');
var tmpl3 = jsRender.templates('./relative.html');

var data = [
  { key : 'value' },
  { key : 3 }
]

var out1 = tmpl1.render(data);
var out2 = tmpl2.render(data);
var out3 = tmpl3.render(data);

console.log(out1);
console.log(out2);
console.log(out3);

console.log(out1 == 'local.htmllocal.html');
console.log(out2 == '/home/joe/website/templates/mainTemplate.html/home/joe/website/templates/mainTemplate.html')
console.log(out3 == 'value\n3\n');

And you get:

local.htmllocal.html
/home/joe/website/templates/mainTemplate.html/home/joe/website/templates/mainTemplate.html
value
3

true
true
true

Since there was no newline at the end of relative.html, there's another error in that the third line of the results really should be "value3" without the newlines.

BUT, the real issue is that the two, valid paths are not recognized.

This is particularly annoying because the path module provides those values directly from path.join and other functions. So, one has to jump through some extra hoops to detect and then mangle path parameters.

[BorisMoore]

Hi,

Thanks for these suggestions. But I am seeing some issues with your proposal - and your pull request:

For the test using path.resolve('.test/templates/name-template.html') - that doesn't work for me because (in Windows) it produces a path which starts with "C:\" or similar, and has a : - so of course fails your regex /^[^\\:*?"<>]*$/.

In fact the design to require paths to follow the canonical pattern starting with "./" was deliberate. It avoids the perf hit of having an unnecessary readFileSync call when you are in fact passing in a template string: jsRender.templates("my template content"). Many of those template strings are likely to pass your modified regex test and trigger the file access, whereas with the "./" design they did not.

Also your design means that there is more than one way to write the path for the same template, so the caching of compiled templates against the name can lead to clones using different lookup keys.

// If the template is not named, use "./some/file.html" as name.`
if (tmpl = $templates[name = name || value]) {

Similarly in scenarios with browserify, the canonical path in my design is also used as a lookup, client-side - see the code in jsrender.js: https://github.com/BorisMoore/jsrender/blob/master/jsrender.js#L773-L781.

With the change, it would lead to different 'spellings' for that id lookup in the browser, and possible duplicates of the client code for the compiled template.

So for the above reasons, I chose to impose the constraint that if you have a path local.htm you have to prepend a "./" (which seems to me to be fairly easy to do...)

BTW for the test where you get console.log(out3 == 'value\n3\n'); I am not getting those newlines. Are you sure you didn't have a newline at the end of your relative.html file?

[jandrieu]

Thanks for checking out my edits. Some further thoughts:

First, you were correct about the newline. I had to use hexdump and apply two different customizations to turn it off, but uknown to me, emacs was appending a newline to the template. Bad emacs.

About the regex and filecheck hit, that's a good argument. I ran some benchmarks, and sadly, I'm not sure it gave me much guidance.

Basically, I ran several functions that could be involved in dealing with variations on the filename format, including the RegEx you use currently, fs.exists, using path functions to coerce from relative to absolute and the reverse, and finally, the cost of checking to see if the value is already in the templates object. I ran the benchmarks against a local file, a relative file, an absolute file, some missing files, some bad files, and a few (in-line) templates.

I've attached my benchmark code as a txt file in case you want to try it yourself.

Here's what I found:

                       local.html  ./relative    /home/joe/. {{template
compiled JSRenderRx     0.054 us     0.070 us     0.050 us     0.051 us
original JSRenderRx     0.062 us     0.081 us     0.059 us     0.060 us
fs.exists               7.753 us     7.789 us     8.290 us    25.09 us
templateLookup          0.101 us     0.093 us     0.129 us     0.098 us
validFileRx             0.067 us     0.054 us     0.051 us     0.051 us
templateRx              0.071 us     0.074 us     0.095 us     0.054 us
coerceRelative         11.15 us     14.04 us     12.60 us     18.14 us
coerceAbsolute          3.690 us     5.462 us     3.889 us     6.985 us

So... you are correct that the regex dominates fs.exists, which is 100x slower. And if you compile it before hand, you'll get another small bump.

However, coercing every path to the relative format you ask for is actually WORSE than fs.exists on my machine. For my use case, my users will be providing arbitrary paths to directories containing collections of templates. That means I'm going to be forced into coercing one way or the other. However, after playing with this, the fastest option is going to be using process.chdir to the directory and using your code as is.

That said--and on a purely academic note--I'm not sure that performance gives us much guidance here. I doubt the template creation is the vital bottleneck, compared to rendering. I know you've thoughts of supporting async methods. That will definitely reshape how the flow works. Also, the possibility of remote templates would be intriguing as well.

It isn't in the version I've already attached, but I added a test for the chdir and that takes .7us, so that is easily the faster option: chdir then rely on your regex approach, prepending './' to all of the files.

Anyway, this was an intriguing exercise. I think my only request would be to update the docs to clarify that the "./" prefix is required for file-based templates.

checkFileTest.js.txt

[BorisMoore]

OK - thanks for digging in further. I have just committed some changes including adding to the docs re: relative path syntax: http://www.jsviews.com/#node/filetmpls.

I'll close this issue.

[dbauszus-glx]

I am finding myself between a rock and a hard place with this limitations.

We are using Zeit Now as platform which does not allow relative paths but only paths defined with __dirname or process.cwd(). Meaning I can either get file or I can use JSRender but not both together.

[BorisMoore]

@dbauszus-glx: In fact the above issue concerns the templates() method, for which you must use relative paths. But for the renderFile() method, you can use absolute paths, as well as relative paths. See https://github.com/BorisMoore/jsrender/issues/353#issuecomment-565688576