Added customizable messages template

[agraboso]

Addresses #11 in a minimal, backwards-compatible way. This pull request adds two arguments to the messages() function, leaving its signature as function messages(template, options) where template and options are exactly as in Express's res.render(template, options). In fact, it uses the latter function internally, and so makes use of Express's handling of templating engines and views.

It works as follows:

1. If req.flash() returns the empty array, messages() outputs an empty string.
2. If req.flash() returns a non-empty array, then:
   • if the template argument is falsy (undefined or an empty string), messages() produces the exact same output as before --- preserving backwards-compatibility;
   • if the template argument is a valid template filename, messages() compiles the template with the given options;
   • if the template is truthy but not a valid template filename, messages() outputs an empty string (i.e., it fails silently).

Tests pass, but I haven't written any new ones --- I'd rather wait and see if this might be merged or not ---, but I have been playing with it, and it works as advertised.

The code draws heavily from @artcommacode's answer in #11.

[artcommacode]

This looks pretty good to me, given docs and new tests I'll definitely merge it!

[agraboso]

I'll get to writing those tests and docs then.

[agraboso]

I couldn't quite figure out the testing framework that was used, so I moved everything to tape+supertest --- which, incidentally, made a couple of files obsolete (.gitignore and Makefile). Let me know if these work for you.

I'll get to the docs presently.

[agraboso]

By the way: the module itself should work in Express 2, but the tests are written using the Express 4 extraction of middleware.

[agraboso]

OK, I'm done. If this is merged, the only thing missing is a version bump in both package.json and History.md.

[artcommacode]

Thanks @agraboso, I'll check it all out when I'm back in the office tomorrow.

[lennym]

This looks pretty sound, I just have one thought... Flattening all of the messages into a single array seems pretty crude, and prevents a user from being able to do anything with the type groups in a template if they wanted to.

If the messages were still passed to the template grouped by type then a user could still use these groupings in in a template (as per the default behaviour), and a flat list (as per the example in this PR) could be implemented as:

<ul class="messages">
<% messages.forEach(function (group) {
    group.forEach(function (message) { %>
        <li class="<%= message.type %>"><%= message.message %></li>
    <% })
}) %>
</ul>

[artcommacode]

(I asked @lennym if they had any opinions on this before I merged your pull request as it's been so long since I've used express-messages myself)

I do agree with them in that it shouldn't move away from the original express-messages layout. My code, which you're reproduced above, is a more dogmatic and less flexible way of doing things and should probably be replaced with something more in-keeping with the original.

[agraboso]

@lennym: I understand your point, and have incorporated it into the PR.

However, there's one thing that may merit a discussion (probably not within the confines of this PR though, since the concern is different and it involves another module): the current connect-flash and express-messages cannot keep the order in which the flash messages were issued. In the example in Readme.md, the error message ("Email delivery failed") is issued after the two info messages ("Email queud" and "Email sent"). We're lucky that Object.keys returns info before error --- neither the ES5 nor the ES2015 specifications mandate a particular order of the strings in the returned array, so this is implementation-dependent.

A more elaborate use case for keeping the order would be something like this:

req.flash('info', 'Email to main address queued');
req.flash('info', 'Email to main address sent');
req.flash('error', 'Email delivery to main address failed');
req.flash('info', 'Email to alternative address queued');
req.flash('info', 'Email to alternative address sent');

which would currently output

<div id="messages">
  <ul class="info">
    <li>Email to main address queued</li>
    <li>Email to main address sent</li>
    <li>Email to main alternative queued</li>
    <li>Email to main alternative sent</li>
  </ul>
  <ul class="error">
    <li>Email delivery to main address failed</li> 
  </ul>
</div>

instead of something like

<div id="messages">
  <ul class="info">
    <li>Email to main address queued</li>
    <li>Email to main address sent</li>
  </ul>
  <ul class="error">
    <li>Email delivery to main address failed</li> 
  </ul>
  <ul class="info">
    <li>Email to main alternative queued</li>
    <li>Email to main alternative sent</li>
  </ul>
</div>

or maybe even the saner

<ul class="messages">
  <li class="info">Email to main address queued</li>
  <li class="info">Email to main address sent</li>
  <li class="error">Email delivery to main address failed</li> 
  <li class="info">Email to main alternative queued</li>
  <li class="info">Email to main alternative sent</li>
</ul>

To be able to deal with this, connect-flash would have to use an array of messages of the type

[ { type: "info", message: "Email queued" }, ... ]

but this would break the backwards-compatibilty of that module --- which may be a more important issue than that of dealing with this use case.

With respect to your concern:

Flattening all of the messages into a single array seems pretty crude, and prevents a user from being able to do anything with the type groups in a template if they wanted to.

Users could still filter messages by type from the flat array by using Array.prototype.filter, but it is true that the template would look a lot more complicated.

Anyway, this is just food for thought.

[lennym]

Users could still filter messages by type from the flat array by using Array.filter, but it is true that the template would look a lot more complicated.

That is only true for template languages that allow for logic embedded in templates, which many do not (mustache for instance), and even when it is supported is considered by many to be "a bad idea".

[agraboso]

That is only true for template languages that allow for logic embedded in templates, which many do not (mustache for instance), and even when it is supported is considered by many to be "a bad idea".

That's true.

[artcommacode]

Okay, looks good to me @agraboso. As good as we're going to get without replacing the dependency on connect-flash with something custom-built anyway.

I'll pass it around to see if there's any more questions or comments and try and get it merged tomorrow. Then we finally get to update express-messages to 1.0.0 so we can be semver compliant too!

[agraboso]

Okay, looks good to me @agraboso. As good as we're going to get without replacing the dependency on connect-flash with something custom-built anyway.

I just found out there's already a module that stores flash messages in a flat array. It's called flash, and it is owned by expressjs on GitHub...

[artcommacode]

Yep, flash does away with both connect-flash and express-messages.

[agraboso]

@artcommacode: any news?

[artcommacode]

Merged, though I've just realised I don't have access to publish it on npm, are you able to help me with that @dougwilson?

[dougwilson]

Looks like only @tj has publishing rights for the express-messages module. If he doesn't respond here from my mention in a few days, I'll talk to him directly :)

[artcommacode]

Thanks Doug!

[dougwilson]

Hey @artcommacode I just sent TJ an email :)

[artcommacode]

Thanks again Doug.

[artcommacode]

@dougwilson: paging you again again to see if you can get in touch with @tj!

[dougwilson]

Hey @artcommacode I tried a couple more times to get @tj and so if he's listening here, @artcommacode is looking to get access to push to the express-messages npm module :)

[tj]

oop, missed that sorry, added now!

[artcommacode]

Thanks @dougwilson, @tj!