vapor / leaf-kit

🍃 An expressive, performant, and extensible templating language built for Swift.
https://docs.vapor.codes/4.0/leaf/getting-started
MIT License
47 stars 38 forks source link

Public visibility for `LeafData.lazy(...)` to build LeafData lazily from a closure #116

Closed esummers closed 1 year ago

esummers commented 1 year ago

Is your feature request related to a problem? Please describe.

I'd love to pull some LeafData in lazily with a closure. It appears to be implemented but disabled. Is this feature hidden due to an incomplete implementation or is it's internal visibility an error?

Describe the solution you'd like

I'd love to see closures for lazy LeafData to become public API.

Additional context

The API I'm referring to is in LeafData.swift:

    /// Creates a new `LeafData` from `() -> LeafData` if possible or `nil` if not possible.
    /// `returns` must specify a `NaturalType` that the function will return
    internal static func lazy(_ lambda: @escaping () -> LeafData,
                            returns type: LeafData.NaturalType,
                            invariant sideEffects: Bool) throws -> LeafData {
        LeafData(.lazy(f: lambda, returns: type, invariant: sideEffects))
    }
esummers commented 1 year ago

If this were to be made public, it would make sense to reorder the lambda parameter to support trailing closures.

tdotclare commented 1 year ago

When I rewrote this part of LeafKit 4, the lazy case was inherited from the previous version - personally I don't consider it a useful thing to make public as creating your own zero-paramter LeafTag adherent encompasses pretty much any comparable use.

The additional parameters besides the lambda are strictly intended to be used internally and must be vetted/exact because they can't be validated by the compiler (EG, a lazy recursively returning a lazy is possible), and improperly representing whether the lazy generator is invariant can corrupt cached ASTs.

For reference, I had originally retained this functionality because it was useful for some potential internal optimizations in cached ASTs for rendering performance, but never intended it to be made public - only for internal, well-vetted cases.

As I'm no longer a maintainer, above is just my opinion but personally I'd remove or comment out for future it so any conditionals that require checking it can be dumped in evaluating LeafData.

esummers commented 1 year ago

Thanks for the history behind this! This was something that could have been good enough for my use case if it was an upcoming feature, but it sounds like it would be best to go in a different direction.

A LeafTag that could act as a lazy for loop generating values for properties enclosed inside would work best for my use case. I want to fetch data that is unknown in advance purely from the template. Unfortunately I don't think that is possible currently. I have a workaround to pre-fetch those values, but it isn't as elegant as I'd like. I'd prefer to keep this all in the template.

Closing this for now.

tdotclare commented 1 year ago

A LeafTag that could act as a lazy for loop generating values for properties enclosed inside would work best for my use case. I want to fetch data that is unknown in advance purely from the template. Unfortunately I don't think that is possible currently. I have a workaround to pre-fetch those values, but it isn't as elegant as I'd like. I'd prefer to keep this all in the template.

Note that as it is currently implemented, Leaf's rendering stage is synchronous. Using tags that are themselves blocking can compromise use, so while less elegant, pre-fetching values is preferred. I assume these are values that are constant for the lifetime of the template, and if that's the case, logically they should be stored in the context passed to Leaf rather than using a tag to so do (IMO).