option to download files rather than view

[coolaj86]

serve-index is great because it allows a quick, generic file viewer.

I've been using it for https://telebit.cloud's directory serving option (telebit http ~/path/to/share) as a poor-man's airdrop/dropbox so that I quickly share files with others.

However, there are two things that make it somewhat inconvenient:

• inability to quickly download a file
• inability to quickly download a group of files

If we had a download icon next to the file that would append ?download=true to the url, then that could be a cue to serve-static to add the appropriate Content-Disposition header so that an image would download rather than go into view mode.

Likewise, if we had the option to save as zip that would append ?download=true&format=zip we could have serve-index provide the file as a zip file (and leave the dependency as optional, simply returning an error if the option in enabled and the dependency is not installed).

I'm wondering if you'd be open to either or both of these suggestions.

[dougwilson]

imo both these suggestions are over complicating both these modules and would tite them together too closely. For example your suggestion that serve-static should do something different if there is a query string parameter.

Adding that header on a query string parameter is already trivial to do in your own code, so to me it sounds like you maybe just want a way to append query strings to the url. Can't this already be done by supplying your own template?

[dougwilson]

Ah, all you get in the template is {files} so you would have to use a template function to do this instead of using the string template. We could make the template use a real engine like ejs or something which would make it more flexible without needed to write quite as much.

The ZIP thing is definitely out of scope of this module -- this module just serves up an index / directory listing, not actual file contents in any way :) Another module could always be made that combines serve index and static of course and builds the features you are describing.

[coolaj86]

I would hate to see this package bloated with a non-optional ejs dependency.

My first thought would be to move the html out of node and back into the html. It would require a breaking change and a major version bump, but most of the other solutions I'm thinking of would too.

I would suggest removing this from createHtmlFileList

  var html = '<ul id="files" class="view-' + escapeHtml(view) + '">'
    + (view === 'details' ? (
      '<li class="header">'
      + '<span class="name">Name</span>'
      + '<span class="size">Size</span>'
      + '<span class="date">Modified</span>'
      + '</li>') : '');

and placing it in the default template as

<ul id="files" class="view-{view}">
  <li class="header header-{view}">
    <span class="name">Name</span>
    <span class="size">Size</span>
    <span class="date">Modified</span>
  </li>

then add header-tiles { display: none; }

then add

.replace(/{view}/, escapeHtml(view))
.replace(/{header}

And then change {files} to

{foreach file}
<li><a href="{dir.path}" class="{classes}" title="{file.name}">
  <span class="name">{file.name}</span>
  <span class="size">{file.size}</span>
  <span class="date">{file.date}</span>
</a></li>
{/foreach file}

which could be extracted very simply with

var tpl = /{foreach file}([\s\S]+){\/foreach file}/gmi.exec(txt)[1];

Another idea would be to beef up the built-in json api so that it returns html-escaped file stat objects and ditch the node rendering altogether in favor of just-as-simple client-side rendering to start with.

I think that convenience + security is the primary benefit of the current way it works. If it could be improved to give greater configurability with even greater convenience and greater security (i.e. not providing non-escaped values to non-security-savvy people creating template functions), that would be a big win.

[dougwilson]

So some of this has been discussed in previous issues, and here is the summary: (1) we don't want to start making an entire template language here, which is what your foreach is proposing. If adding a dep on a template engine is too much, then we should just remove the string template function all together and users and use the function template instead. And (2) providing html escaped in json reaponse is utter nonsense. That will never happen here. You need to escape in the client side where you know you are inserting into html unsafely.

[dougwilson]

Also note that the view provided out of the box here is designed to work in browsers that lack css and javascript support, so whatever the final solution is needs to continue to work without client side javascript.

[dougwilson]

(i.e. not providing non-escaped values to non-security-savvy people creating template functions

This issue is more easily solved by not reinventing a new template language in the first place and instead using a template engine people know, can find tutorials on, provides default escaped interpolation etc.

[dougwilson]

And ejs was just a suggestion; we can use any engine that meets whatever the minimum things we need to provide are (sounds like you're suggesting html escaping and bock iteration as a needed things the template engine should support).

[dougwilson]

Also: a major version bump is fine. This module needs to be refactored as is without even adding new features, mostly around the fact that most settings are global instead of per instance. So making suggestions that are breaking changes is fine, and whatever lands would of course have a migration guide written along with it so it's no big deal.

[coolaj86]

I think it would be nice to move the renderer out into it's own module, say serve-tpl-default and encourage others who publish their own variations to do so with some sort of prefix like serve-tpl- so that it would be easy to find via an npm search like https://www.npmjs.com/search?q=serve-tpl-.

When I first started looking at using the template string the reason was that I just wanted to make an addition without having to go figure out my own icon set from scratch (I'm not much of a css guy), and I didn't want to create and publish a full fork.

And I don't know how much other people would want to customize this rather than just create their own static server, but things I would like to be able to do if it were expanded:

• outer html (the broad look, currently changed via template string)
• inner html (the single tile or list item, currently embedded in the node)
• style
• javascript
• pngs (add to or replace the list of file extensions if I know I have something special I'll be showing)

If all of those were opened up for replacement I don't think a template system would be necessary.

In any case, I'd prefer to have access to what's already there so that I could just use the existing icon set or stylesheet and append to the array or the string.

P.S. By html-escaped json responses I meant showing "/path/to/wierd&lt;thing&gt;" instead of "/path/to/wierd<thing>", not full html templates. I see the argument for not doing that, but nothing is secure until it's more convenient for the most ignorant end user of the product to do the right thing than to do the anything else. Therefore whatever can be done to make it more convenient for people to use stuff that has the security baked-in wins in my book.

Also, there's a few unusual, but probably negligible, things this module does that might be worth changing if this is refactored:

• synchronous reads for images (but only once, not that bad)
• embeds the same image multiple times, in full (but they're small, so not that bad) (it's just once in the css file, I misread)
• stats objects for html, but not for json or plain text (disappointing if you want to use the json to construct a template)

[dougwilson]

A lot of this module was inherited by me and so I don't knows the ins and outs as to why things have been done. And then most of the current functionality was contributions by pull requests, so whatever is lacking is because the user who made the pull requests likely did not need that specific feature.

I know there are a lot of things listed here. Perhaps we should split it out into individual action items you or I can start to take a stab at in a pull requests one by one? What do you think should be the first action item, if you were to pick something from the list?

[coolaj86]

I'd put them in this order:

• merge any existing PRs that are ready (you)
• update style guide and existing code to match (you)
• move lookup for stat objects for files so that they can be used in each of serveIndex.plain, serveIndex.json, and serveIndex.html (I can do this easily) stat files before serveIndex, also output date, size, and type #74
• change createHtmlRender to accept object with all of the necessary template strings (I've already done this in a fork)
  • page (mostly as-is)
  • list
  • header
  • file
  • style (mostly as-is)
  • javascript
• checkpoint for review (you)
• move renderer to its own file, if desired (me)
• export default icons, templates, etc from renderer file (me)
• publish renderer as separate module, if desired (you)

I believe making things per-instance rather than global will be a natural part of that process.

[dougwilson]

That sounds good. I'm not sure what the purpose of the checkpoint one is since each thing would be a pull request anyway :)

The renderer points I don't understand. This org has a policy that each module has only one file: index.js . So unless the had more files before that (this one didn't) we want to only have a single file index.js. the purpose is to encourage code sharing, where if it's too big for one file it should be a separate module.

I, myself, am not interested in maintaining yet another module, though. So if you think tbe outcome of this means a new module, you'll need to maintain and publish it or find another volunteer to do so 👍

[coolaj86]

The checkpoint because each change depends on the prior change and I don’t want to continue far down a path without your OK. I want to make sure this effort results in a merge and not a fork.

I don’t think the render file is too large, but I think that it deserves a separation of concerns (security & function vs view & style) and I’m hopeful that separating the render out will lead to fewer half-baked forks and more, usable “theme” modules.

Would you mind creating a "v2.0" or “next” branch that I can target for PRs since this will include breaking changes to the plain and json views as well as the template function signature?

I’ll go ahead and start preparing a few PRs.

[dougwilson]

Yep, I will make a branch after I get those PRs merged and the style changes (from the checkboxes above) so the changes will be based on something that is mergable (changing the entire style will cause any outstanding pull requests unmergable when it happens, per the conversation in the other issue). If I made the branch now before that lands, then the branch becomes useless since it cannot merged back into master after the style change 👍

[ghuser]

I would like to propose a simple feature-request for the specific issue:
Why not add a new option {downloadAttr: true} which would create links with the download attribute?

As a workaround for now, you can copy use the existing directory.html template ( {template: './directory.html'} ) adding the following at the end of body tag:

<script>document.querySelectorAll("#files a").forEach((el)=>el.setAttribute("download", ""));</script>

[AverTry]

I would like to propose a simple feature-request for the specific issue:
Why not add a new option {downloadAttr: true} which would create links with the download attribute?

As a workaround for now, you can copy use the existing directory.html template ( {template: './directory.html'} ) adding the following at the end of body tag:

<script>document.querySelectorAll("#files a").forEach((el)=>el.setAttribute("download", ""));</script>

Needed this, but it tries to download directories too, my work around is

<script> document.querySelectorAll("#files a").forEach( (el) => { if(!el.classList.contains('icon-directory')) { el.setAttribute("download", "") } }) </script>