express icon indicating copy to clipboard operation
express copied to clipboard

`ext` should be case-insensitive for view engines.

Open issuefiler opened this issue 3 years ago • 3 comments

Actually, in issue https://github.com/expressjs/express/issues/4593, I provided Express the imported view engine instance. I registered it as "Eta" (".Eta" internally), and Express did not use it for search.eta (because it was only looking for the ".eta" one), triggering the undesired automatic require.

app.engine("Eta", Eta.renderFile);
console.log(app.engines); // { '.Eta': [Function: renderFile] }
app.set("view engine", "Eta");
function View(name, options) {
  var opts = options || {};

  this.defaultEngine = opts.defaultEngine;
  this.ext = extname(name);
console.info("EXPRESS", [name, extname(name)]); // EXPRESS [ 'search.eta', '.eta' ]
  this.name = name;
  this.root = opts.root;

Maybe it should normalize the ext to a single case?

https://github.com/expressjs/express/blob/5c4f3e7cc76fed9b42c27cebcdd9d66ef63092f9/lib/application.js#L293-L307

https://github.com/expressjs/express/blob/5c4f3e7cc76fed9b42c27cebcdd9d66ef63092f9/lib/view.js#L52-L88

issuefiler avatar May 17 '21 17:05 issuefiler

Hm, that is a good point. I don't think such a change would be backwards compatible, but it could potentially be changed in a future major version of Express. I would like to leave this open to explore if folks are using the case sensitivity or not before we should decide on the change. Most file systems are not case insensitive.

dougwilson avatar May 17 '21 17:05 dougwilson

@dougwilson how would it not be backwards-compatible, except if a user foolishly specified 2 same-name view engines with only casing as a difference? Bumping as it may be a good time to take a decision on this issue with the v5 beta's? To me it feels logical, just as an OS treats ..JPG & .jpg as the same ext. But I would only support it if the toLowerCase is only required at the view engine definition level (not for each template)

webketje avatar May 10 '22 10:05 webketje

Hi @webketje thanks for your input. We are trying to refrain from continuing to snowball the 5.x release with endless changes, and ideally just the last few on the list are all that is left. The change can just be targeted to the next major.

As for why it is not backwards compatible, as stated, most file systems (particularly on servers) are not case insensitive, and so you can actually have both a index.foo and a index.Foo file. Unfortunately we do not have insight in to all the ways folks use Express, and we have many examples and experience that a change like this will break folks. Since we value a stable API where you don't have to worry about upgrading Express, this is a breaking change, as it would change the behavior of view engine extension registration.

dougwilson avatar May 10 '22 16:05 dougwilson