Fix behavior of Leaf context encoding and improve Leaf errors #212
Conversation
…to Codable requirements, and actually support superEncoder(). Add a bunch of missing test coverage.
… remove unnecessary usage of _openExistential(), remove some unneeded throws and optionals
|
this pull request is great, but I found a bug. 1: add a route like this, and pass empty list to todoList parameter func routes(_ app: Application) throws {
app.get { req async throws -> View in
let context = DemoModel(title: "Demo", todoList: [])
return try await req.view.render("demo", context)
}
}
struct DemoModel: Encodable {
let title: String
let todoList: [String]
}2: in leaf template file, use #count() tag to check the todoList count when visit this page on browser will encounter error: {
"error": true,
"reason": "Unable to convert count parameter to LeafData collection"
}and I tested leaf 4.2.0, it work right! |
|
cc @gwynne looks like some behaviour has changed here |
|
Yeah, to say the least... I'll add some more tests to cover whatever this turns out to be. |
Sources/Leaf/LeafEncoder.swift
Outdated
| let encoder: EncoderImpl | ||
| var data: [LeafEncodingResolvable] = [] | ||
| var nextCodingKey: CodingKey { GenericCodingKey(intValue: self.count) } | ||
| var resolvedData: LeafData? { let compact = data.compactMap(\.resolvedData); return compact.isEmpty ? nil : .array(compact) } |
There was a problem hiding this comment.
Was this intentional? It now means that in templates I need to do:
#if(myArray):
#if(count(myArray) > 0):
…
#endif
#endif
Which seems to be a regression of how templates are written.
Sources/Leaf/LeafEncoder.swift
Outdated
| private final class EncoderKeyedContainerImpl<Key>: KeyedEncodingContainerProtocol, LeafEncodingResolvable where Key: CodingKey { | ||
| let encoder: EncoderImpl | ||
| var data: [String: LeafEncodingResolvable] = [:] | ||
| var resolvedData: LeafData? { let compact = self.data.compactMapValues { $0.resolvedData }; return compact.isEmpty ? nil : .dictionary(compact) } |
There was a problem hiding this comment.
Same issue here as with arrays below.
An empty array or dictionary is an intentional choice, and I don't think Leaf should be eliding it, given that it requires double checks.
We also can't do something like this:
#if(myArray && count(myArray) > 0):
…
#endif
Because of vapor/leaf-kit#85
|
Nope, this is a bug. I'll be pushing a fix for it shortly. |
…ields+Codable conformance with regards to empty containers: They must be explicitly preserved.
…ing, don't use a separate class for single value containers (should make encoding of complex contexts a little faster), simplify some names, remove unnecessary use of _openExistential() from the tests
|
That should do it 🤞 |
There was a problem hiding this comment.
This is working well with the latest changes - thanks, @gwynne! ❤️
|
These changes are now available in 4.2.1 |
Types which use
superEncoder(forKey:)in theirCodableconformance (such as Fluent models) can now safely be provided to Leaf views as context objects. In general, handling of context encoding is significantly improved.LeafErrornow conforms toAbortErrorandDebuggableErrorfor improved error UI/UX.