Resolve paths for views asynchronously

[aredridel]

This eliminates the only post-startup synchronous I/O in Express.

In addition, this makes the view.lookup method non-private, intended to be used
by view engines to resolve paths to partials, and, if the View class is
extended, could be used for other resources such as internationalized messages.

[dougwilson]

Nice! Anyway we can add to this a way to prevent stampeding (which the sync version protected from) to the default lookup implementation?

[aredridel]

Oh, yes! I think I could work that in.

[aredridel]

Updated.

[gabrielcsapo]

👍

[bjohansebas]

Hey, @aredridel could you rebase this PR, please?

[aredridel]

Will do later today hopefully

[wesleytodd]

It is just lovely when a 10 year old PR with value is revived! This might not make it into the 5.1.0 release, but we will be sure to review and merge it ASAP after. Thank you for being patient and then responsive after so long!

[aredridel]

Isn't it just? I get so mad when projects close things after 30 or 180 days as stale. Sometimes we should take our time!

Anyway, rebased. I did not keep the view options being passed to the lookup function. That's a little useful, I guess, but not really needed.

[wesleytodd]

I just merged the last required change we needed to land before cutting the 5.1.0 release branch. If this is ready now I can review it and see if landing it for the release is an option.

[wesleytodd]

The fact that we have no direct coverage for the View class makes me concerned. The changes here, if made to a public api, would be breaking. The only ways this is exposed to consumers is via the view setting, which is not documented, or via a deep import to lib/view.js. In theory someone could do:

const View = app.get('view');
const view = new View();
const path = view.lookup('name');

And this would break with this change. In some past cases we have been careful enough to avoid even this internal concealed breakage. That said, I am actually a fan of us moving a bit toward keeping our internals safe and then breaking them more often. That is one of the only ways to really prevent folks from coupling to them. So with that in mind I personally would love to land this for the 5.1.0 release but will need to get more consensus before we do. This might mean it doesn't make the timeline, but 🤷 what's another 10 years?

Either way, pinging in @expressjs/express-tc just in case anyone has an immediate and strong opinion.

[bjohansebas]

LGTM!

[bjohansebas]

The only ways this is exposed to consumers is via the view setting, which is not documented,

I don't have a strong opinion on this, I'd rather not say anything.

via a deep import to lib/view.js

If someone is doing this, they really should stop. The location of that file could be deleted or moved, which would affect their application, and I don’t think we should worry about that.

By the way, we should start testing the views whenever possible.

[dpopp07]

That said, I am actually a fan of us moving a bit toward keeping our internals safe and then breaking them more often. That is one of the only ways to really prevent folks from coupling to them. So with that in mind I personally would love to land this for the 5.1.0 release

Agreed. This would focus compatibility with the contract, which I think is good. Users can always figure out creative ways to use the library in ways it wasn't intended to be used (like deep imports) 🙂 but I'm fine with breaking those.