Allow path functions to be async about inert

[sholladay]

Being easy doesn't mean that it is right.

To me, it's easy and far more right. Allowing path to be a function shows a recognition that there are valid use cases where the path cannot be known ahead of time and needs to be computed at request time. Whether that resolution is synchronous or asynchronous should be of no concern to Inert, that's an implementation detail on the user's side. The fact that my app has been working well for years using this feature but now needs a slight tweak to be async doesn't suddenly make that use case invalid. Can we agree on that? If so, then it should be supported using the same mechanism. In my opinion, using pre is just a hack. If you strongly believe in using pre for providing paths dynamically, then let's remove support for path functions and instead have an option for a lookup key where Inert will get the path from request.pre. Then it will at least be consistent.

workaround for the rare cases that you want async.

As I tried to explain, this has to do with the move to ES modules. There's a massive ecosystem shift underway. This is about to be much more common.

Looking up stuff is one of the use-cases that pre handlers are designed for.

Don't have path functions, then.

Otherwise what is the point of supporting assignment of the result to a request variable?

I don't know. I've never needed pres and I've worked on a lot of hapi projects for many years. They only make the code more complicated. I avoid them, especially in large applications. I can't tell you how many problems and confused developers I've seen struggle with contextual objects that are mutated and invisibly passed between functions. hapi's decorate() functions have the same issue, albeit they are more useful in my experience. I really hope that hapi continues to move towards functional programming and away from things like pre and decorate().

[sholladay]

Good idea. I'm fine with that solution. Thanks, Devin.

[thefoxie]

@sholladay is right. I've tried to use path function, but all I found out was those lines that don't allow me to use async function. I cannot do that differently as I have paths stored in DB and store them on whatever directory structure is used at the time (actually cannot even know beforehand), but serve them from one route (again, actually not one route as there's another logic, but you get the point).

[kanongil]

I would not support doing that.

Rather, you can solve your issue by adding a pre, where you do the resolve, and then using that request.pre.whatever value for your path. The request is already exposed in the callback.

[sholladay]

That seems like a needless layer of indirection. I think I'd rather find a way to keep this file as a CommonJS module just so I can have synchronous require.resolve() than to use a pre and share state between functions like that.

Also, the directory handler is already async... All that's needed is to change this line from this...

const paths = normalized || internals.resolvePathOption(settings.path.call(null, request));

... to this ...

const paths = normalized || internals.resolvePathOption(await settings.path.call(null, request));

Error handling is already in place and this would not be a breaking change.

The same principle also applies to the file handler.

[kanongil]

Being easy doesn't mean that it is right.

I don't see why we should add async path lookups after all this time, when there is a relatively accessible workaround for the rare cases that you want async.

Looking up stuff is one of the use-cases that pre handlers are designed for. Otherwise what is the point of supporting assignment of the result to a request variable?
Doing the logic in a pre handler also has the advantage of access to the h toolkit.

[kanongil]

Being easy doesn't mean that it is right.

You used being simple to implement as a primary argument for adding this feature, when it is merely a byproduct.

path functions are exposed as an option for cases where the path needs to be computed based on some runtime / request variables. It is not intended as a place to do actual work. A function already allows more than is intended, so the implementation is a compromise favoring ease of implementation, simplicity, and performance at the expense of some developer discipline when implementing the path function logic. Eg. don't mutate state.

As such, handling promises is outside the scope of what you are supposed to do.

workaround for the rare cases that you want async.

As I tried to explain, this has to do with the move to ES modules. There's a massive ecosystem shift underway. This is about to be much more common.

I find it hard to believe that looking up module paths inside a path function is anything but a rare use case.

[devinivy]

I think you both bring up good points. I wonder if a reasonable compromise would be to introduce h.directory() so that a handler can be implemented with arbitrary logic to serve a directory. This would be the canonical way to do non-trivial work prior to serving a directory, and the directory handler-type path config would exist for convenience. I think this would keep intended usage consistent with h.file() versus the file handler-type, bearing in mind the file handler-type has an analogous path configuration.

[devinivy]

Sure thing! I've created a new issue and we can continue conversation about that feature over there: #155

[kanongil]

I acknowledge that there is an issue here, but I still don't believe that the solution is to enable async path functions. Maybe the better solution is what @sholladay proposed here:

Looking up stuff is one of the use-cases that pre handlers are designed for.

Don't have path functions, then.

I suggest that dynamic paths can be enabled by a new lookup option, that would be required when no path is provided. This would take a string / array with a path to a property on the request object (as per the second option to Hoek.reach()).

Then inert just calls Hoek.reach(request, options.lookup) to retrieve the path for the current request. Custom app logic would probably have to go in a pre handler. Many cases could probably be resolved without a pre, eg. by pointing to an auth property like auth.artifacts.path.

How does that sound?