readPartials overwrites data in options.partials

[mrighele]

I am using the library with mustache. Since the partials I use are shared among many pages, I have tried to keep them all together in a global object, writing something like the following:

const partials = {
    header: "partials/header",
    footer: "partials/footer",
    /* ... */
}

/* ... */

app.get('/', function(req, res) {
    res.render('index',  { partials });
});

With this approach, the first call to the render function will change the values the partials global to the content of their respective files. This means that the call will work properly the first time, but fail from the second onward, since it will try to load a file with random html as a name. I think this result is not what a user would expect, and if not changed it should at least be mentioned in the documentation (I didn't see any reference to it).

My current workaround is to call a function returning the literal object instead, but it feels like a dirty hack, suggestions for a better approach would be appreciated.

[doowb]

@mrighele thanks for the issue. I've noticed this before, but haven't had a chance to address it. If you'd like to do a PR with code changes to improve it, or a mention on the README.md, that would be appreciated.

For now, you should be able to make a shallow clone of the partials object:

const partials = {
  header: "partials/header",
  footer: "partials/footer",
  /* ... */
};

/* ... */

app.get('/', function(req, res) {
  res.render('index',  { partials: Object.assign({}, partials) });
});

This is similar to your function approach.

[mrighele]

I will prepare a PR, after giving a check to the code it doesn't seems too difficult to fix. What kind of solution do you prefer ? I have two possibilities in mind. The first is to to make a shallow copy of both the options object and the partials element before modifying it. It is a simple solution, but this means potentially cloning an object with lot of keys so I would have to do some tests to see if this change has any impact. The alternative is not to overwrite the original partials element and instead pass around an extra object with the loaded partials. I would prefer the latter approach as it seems to be more clean.

[doowb]

Since consolidate isn't doing a shallow clone of the options currently, I agree with your second option. I don't think that we need an extra object to be passed around, but instead, the readPartials method can pass a new partials object back through the callback. Then in each engine's render implementation, that new object can replace the one on the original options. This might be what you meant, but I initially read that as passing an extra object to readPartials.

It looks like there are only 3 engines that use the partials object now (react only deletes it). If you submit a PR we can discuss code specifics in more details there.

Thanks!