add render-time and async node error recovery pattern#802
add render-time and async node error recovery pattern#802tmarmer wants to merge 45 commits intoerror-controllerfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## error-controller #802 +/- ##
====================================================
+ Coverage 86.02% 86.36% +0.34%
====================================================
Files 513 513
Lines 23566 24029 +463
Branches 2703 2828 +125
====================================================
+ Hits 20272 20753 +481
+ Misses 2956 2942 -14
+ Partials 338 334 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 123.04kB (2.12%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: plugins/common-types/coreAssets Changed:
view changes for bundle: plugins/check-path/coreAssets Changed:
view changes for bundle: react/playerAssets Changed:
view changes for bundle: plugins/beacon/coreAssets Changed:
view changes for bundle: plugins/async-node/coreAssets Changed:
view changes for bundle: react/subscribeAssets Changed:
view changes for bundle: plugins/common-expressions/coreAssets Changed:
view changes for bundle: plugins/reference-assets/coreAssets Changed:
view changes for bundle: plugins/markdown/coreAssets Changed:
view changes for bundle: plugins/stage-revert-data/coreAssets Changed:
view changes for bundle: core/playerAssets Changed:
view changes for bundle: plugins/metrics/coreAssets Changed:
|
|
/canary |
| /** Returns a function to be used as the `replacer` for JSON.stringify that tracks and ignores circular references. */ | ||
| const makeJsonStringifyReplacer = (): ReplacerFunction => { | ||
| const cache = new Set(); | ||
| return (_: string, value: any) => { | ||
| if (typeof value === "object" && value !== null) { | ||
| if (cache.has(value)) { | ||
| // Circular reference found, discard key | ||
| return "[CIRCULAR]"; | ||
| } | ||
| // Store value in our collection | ||
| cache.add(value); | ||
| } | ||
| return value; | ||
| }; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Why do we need to do this? Shouldn't internal Player state be serializable?
There was a problem hiding this comment.
The issue here stems from the ResolverError adding in the failed node in its metadata. We need a better solution but this was put in place to allow the metadata to get logged without throwing errors since the node itself has circular references.
If we don't want the node logged or set in the dataController as part of captureError we either need to not include the metadata in logs/data or we need another mechanism to identify resolver nodes with to avoid sending the whole object to keep its reference.
There was a problem hiding this comment.
So this is so that the error includes the originating AST node, gotcha. I think a reference should be able to be stored fine in the Data Controller without serialization right? For logging can we just reference the asset ID?
There was a problem hiding this comment.
If the error controller was aware of the metadata being logged we could, but because any error with any metadata could pass through it, we can't identify when we're getting something like a Node or a simpler object. That and sometimes the originating Node being logged might be an async node or something else that generated the asset.
| export const ResolverStages = { | ||
| ResolveOptions: "resolve-options", | ||
| SkipResolve: "skip-resolve", | ||
| BeforeResolve: "before-resolve", | ||
| Resolve: "resolve", | ||
| AfterResolve: "after-resolve", | ||
| AfterNodeUpdate: "after-node-update", | ||
| } as const; |
There was a problem hiding this comment.
Is there a way we can automate this?
There was a problem hiding this comment.
Couldn't find a way to automatically form this object, but simplified the types here to use an enum. If you have any ideas here I'm open to try something else
…if no transition available.
@nancywu1 The only example I add here is an update to the |
I added doc and tests for error controller. There weren't specific storybook/demo app examples for controllers, but since this one added a navigation errorState, so may worth adding it to storybook like managedPlayer. Thought? |
jvm/core/src/main/kotlin/com/intuit/playerui/core/bridge/JSErrorException.kt
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| if (exception is AssetRenderException) { | ||
| exception.assetParentPath += assetContext |
There was a problem hiding this comment.
i'm a bit late here, this looks like it's supposed to bubble up from the asset level that threw the original error to show the full path? Does this mean the final message is going to be in reverse order from the originating asset?
There was a problem hiding this comment.
The list will be in reverse-order of the actual view tree, but this is used to create the error message here. The reverse order lets us list the most relevant asset first and list where it and its parents were found in.
| styles: AssetStyle? = null, | ||
| tag: String? = null, | ||
| ) { | ||
| val assetTag = tag ?: asset.id |
There was a problem hiding this comment.
what's this tag logic change
There was a problem hiding this comment.
The code here was originally calling withTag on the asset context even if no tag was provided. This change was meant to prevent that, but I didn't want to change the .testTag call to avoid breaking tests. This was something @sugarmanz and I noticed when I was testing earlier iterations where the I was matching assets based on the assetContext.id but was getting the asset id duplicated in the string. This is out of scope of the PR though so if there are issues with this I can revert it and we can look into this more in depth another time.
...main/kotlin/com/intuit/playerui/core/bridge/serialization/serializers/ThrowableSerializer.kt
Show resolved
Hide resolved
| ) | ||
| Assertions.assertEquals("ErrorType", exception.type) | ||
| Assertions.assertEquals(ErrorSeverity.ERROR, exception.severity) | ||
| Assertions.assertEquals(mapOf("testProperty" to "testValue"), exception.metadata) |
There was a problem hiding this comment.
following previous comment, i'm a bit surprised this is passing, @sugarmanz what am i missing
There was a problem hiding this comment.
As per my other comment, I think GenericSerializer was picking the MapSerializer for me. I've updated ThrowSerializer to use MapSerializer instead.
| guard | ||
| let flow = value?.objectForKeyedSubscript("flow"), | ||
| let message = value?.objectForKeyedSubscript("error")?.objectForKeyedSubscript("message")?.toString() | ||
| let err = value?.objectForKeyedSubscript("error") |
There was a problem hiding this comment.
Could we put all keys in an enum, not string literals in the code. I've noticed this pattern across several iOS files (CompletedState.swift, NavigationStates.swift, PlayerControllers, etc.): like objectForKeyedSubscript("flowResult"), objectForKeyedSubscript("controllers") etc. A typo in any of these would silently return undefined with no compiler warning. If it's out of scope for this PR, I'd be happy to take on a follow-up PR to address it.
There was a problem hiding this comment.
I'll make the keys for this object into an enum, but could you make a github issue to follow up and remove this across the repo? I think it's a good practice we should try to keep up with
| return jsError.getJSErrorObject() | ||
| } | ||
|
|
||
| let errObj = objectForKeyedSubscript("Error").construct(withArguments: [error.jsDescription]) |
There was a problem hiding this comment.
The key "Error" in objectForKeyedSubscript("Error") (the JS global Error constructor) vs "error" in objectForKeyedSubscript("error") (a property on the state object) are easy to confuse at a glance. As a newcomer to the codebase, I had to look them up to confirm they're intentionally different.
Would it be worth extracting JS keys into named constants? For example:
private enum JSGlobal {
static let error = "Error" // JS Error constructor
}
This way, objectForKeyedSubscript(JSGlobal.error).construct(...) makes the intent immediately clear without needing to know the JS convention. Not a blocker — just a readability thought for future contributors.
There was a problem hiding this comment.
I like this idea a lot! I'm wondering what you think of taking it maybe one step further and adding a utility to shortcut constructing known JS classes from the JSContext. Thinking something like:
internal enum JSClass: String {
case Error = "Error"
}
internal extension JSContext {
func getJSClass(_ name: JSClass) -> JSValue? {
objectForKeyedSubscript(name)
}
func constructClass(_ name: JSClass, withArguments: [Any]?) -> JSValue? {
getClass(name).construct(withArguments: withArguments)
}
}It's definitely on the simpler side of utilities but imo it helps clear up the intent of the enum since I don't think it's immediately obvious that the use is meant for objectForKeyedSubscript on the JSContext itself.
|
|
||
| public protocol ErrorWithMetadata : Error { | ||
| var hasMetadata: Bool { get } | ||
| var type: String { get } |
There was a problem hiding this comment.
Nit: since errorType is a String to allow plugin-defined custom types, consider adding a doc comment on captureError(errorType:) noting that callers should prefer ErrorTypes constants when applicable, but custom strings are accepted for plugin-specific error types.
| return type | ||
| default: | ||
| return "UNKNOWN" | ||
| } |
There was a problem hiding this comment.
"UNKNOWN" for non-metadata cases — should this be an ErrorTypes constant? like add ErrorTypes.unknown.
There was a problem hiding this comment.
Removed the "UNKNOWN" case. The fallback now is to use an empty string to represent not having a known error type. This should only happen for errors that don't follow our shared interface so I think it's okay to keep this out of ErrorTypes enum imo.
KVSRoyal
left a comment
KVSRoyal
left a comment
There was a problem hiding this comment.
Just a few really small comments. Otherwise lgtm
| public enum TryCatchResultKeys { | ||
| static let success = "success" | ||
| static let result = "result" | ||
| } |
There was a problem hiding this comment.
- Nit: Could this conform to
Stringjust like theJSClassenum. It's also weird this enum doesn't have any cases 😅 so it would be a good change in that regard too. E.g.
public enum TryCatchResultKeys: String {
case success, result
}- Does this need to be
public? They seem like an internal implementation detail. If they don't need to be public, can we remove the access modifier (will default tointernal)?
| public enum JSKeys { | ||
| static let message = "message" | ||
| static let type = "type" | ||
| static let severity = "severity" | ||
| static let metadata = "metadata" | ||
| } |
There was a problem hiding this comment.
Same comment here about the enum. Can this conform to String and have cases?
|
|
||
| extension JSValueError: ErrorWithMetadata { | ||
| public var hasMetadata: Bool { | ||
| !type.isEmpty |
There was a problem hiding this comment.
is it correct for hasMetadata to check type is empty?
There was a problem hiding this comment.
Because the ErrorWithMetadata requires the type property, it's the simplest identifier we have for errors coming from JS. I could alternatively make hasMetadata a property and set its value during init based on the existence of type coming from JS. This would allow empty strings as a valid type value and make the condition a little more. Would that work better here?
| } | ||
|
|
||
| public protocol ErrorWithMetadata : Error { | ||
| var hasMetadata: Bool { get } |
There was a problem hiding this comment.
hasMetadata is purely derivable from metadata != nil. It should not to be a protocol requirement
There was a problem hiding this comment.
Maybe the property name is incorrect. I need a way (at least on the JSValueError to declare whether or not the error actually conforms to the shared interface error with metadata interface. The only required property for that interface is the type property (which is why I check if it's empty on JSValueError) but the others are optional. I figured having this extra property would also allow enum errors to only have the additional data supported on some error types.
There was a problem hiding this comment.
I think hasMetadata should not be in the protocol. you could probably do this:
func error<E>(for error: E) -> JSValue? where E: Error, E: JSConvertibleError {
if let jsValueError = error as? JSValueError {
return jsValueError.originalJSError
}
let errObj = constructClass(.Error, withArguments: [error.jsDescription])
if let errorWithMetadata = error as? ErrorWithMetadata, let err = errObj { /// this is checking if it is nil
if !errorWithMetadata.type.isEmpty { /// this is checking if the type is empty
err.setValue(errorWithMetadata.type, forProperty: JSValueError.JSKeys.type)
}
if let severity = errorWithMetadata.severity {
err.setValue(severity.rawValue, forProperty: JSValueError.JSKeys.severity)
}
if let metadata = errorWithMetadata.metadata {
err.setValue(metadata, forProperty: JSValueError.JSKeys.metadata)
}
}
A separate flag that can drift out of sync with the actual properties. In another place
extension AssetRenderError: ErrorWithMetadata {
public var hasMetadata: Bool {
true
}
it's boilerplate forced by the protocol that adds no value. The protocol shouldn't require conformers to implement a property that's always a constant or always derivable from other properties already in the protocol.
There was a problem hiding this comment.
Alright, makes sense to me. I think I could skip the isEmpty check within the error function since it really is just an edge case for JS errors coming from the core layer. In those cases the first check that returns the original object anyway means we don't even really need to care when serializing the error since anybody else conforming to that protocol should be provided some kind of recognizable string value anyway. I'll still leave a isErrorWithMetadata on JSValueError itself in case we need some way to tell whether or not there is any meaningful metadata on the error (like if somebody is tapping into the onError hook on the error controller)
| This class represents a JS Hook with the ability to return a result. This can work for a Waterfall or Bail hook. | ||
| */ | ||
| public class BailHook<T>: BaseJSHook where T: CreatedFromJSValue { | ||
| public class HookWithResult<T, R>: BaseJSHook where T: CreatedFromJSValue, R: Encodable { |
There was a problem hiding this comment.
May you share the reason to add Encodable here?
There was a problem hiding this comment.
This file is for tapable hooks that interact with the JS layer. It's possible that the result is read by some core JS code so unless I misunderstand how the integration works, it would need to be Encodable in order to actually work.
There was a problem hiding this comment.
I think you might not need Encodable. Encodable implies encode(to:) is used for JS serialization, but it isn't — JavaScriptCore bridges via Obj-C (NSNumber, NSDictionary, etc.).
aka: When a @convention(block) closure returns a value to JavaScriptCore, it doesn't know about Swift's Codable system. It uses Objective-C bridging.
9 participants
captureErrorto fail the player state if no error transition is available.PlayerViewModel.failwhich now uses the error controller.PlayerErrorMetadatainterface for errors to implement to include info required for the error controller to capture errorsJSValueErrorto support serialization with JSValue. UpdateErrorStateto useJSValueError.toInvokableto fix an issue where functions returning arrays of objects would deserialize those objects into aV8Nodeeven if adeserializationStrategywas available for it.Change Type (required)
Indicate the type of change your pull request is:
patchminormajorN/ADoes your PR have any documentation updates?
Release Notes
AsyncNodePlugin'sonAsyncNodeErrorhook to depend on theErrorController'sonErrorhook for finding errors.ErrorController.captureErrorand allow for error recovery on all platforms.PlayerErrorMetadatainterface across platforms and ensure serialization of errors keeps additional metadata.ErrorController.captureErrorwill prefer data from the error object when calling theonErrorhook.📦 Published PR as canary version:
0.15.1--canary.802.31569Try this version out locally by upgrading relevant packages to 0.15.1--canary.802.31569