-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Playa 8756 - iOS add callTryCatchWrapper function on JSValue #270
Conversation
let isError = self.errorCheckWrapper?.call(withArguments: [result as Any]) | ||
|
||
if isError?.toBool() == true { | ||
throw JSValueError.thrownFromJS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we include error information about the underlying JS Error?
""" | ||
(fn, args) => { | ||
try { | ||
console.log(args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.log(args) |
console.log(args) | ||
return fn(...args) | ||
} catch(e) { | ||
console.log(e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.log(e) |
return fn(...args) | ||
} catch(e) { | ||
console.log(e) | ||
if (e instanceof Error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should always be true, so its probably not needed
- args: List of arguments taken by the function | ||
*/ | ||
public func callTryCatchWrapper(args: Any...) throws { | ||
let result = self.tryCatchWrapper?.call(withArguments: [self, args]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since tryCatchWrapper
is a computed property, that means it evals that snippet every time we call this
does JavascriptCore clean it up automatically correctly? or should will we need to handle something?
- parameters: | ||
- args: List of arguments taken by the function | ||
*/ | ||
public func callTryCatchWrapper(args: Any...) throws { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this name is a bit long, can we just call it tryCatch
maybe ?
- parameters: | ||
- args: List of arguments taken by the function | ||
*/ | ||
public func callTryCatchWrapperWithReturnValue(args: Any...) throws -> JSValue? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could potentially just use this one and mark it @discardableResult
since a Void
function in JS would still return undefined
technically
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #270 +/- ##
==========================================
+ Coverage 81.87% 82.16% +0.28%
==========================================
Files 134 132 -2
Lines 4608 4546 -62
Branches 1138 1119 -19
==========================================
- Hits 3773 3735 -38
+ Misses 552 534 -18
+ Partials 283 277 -6 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
for functions not called as a part of a player process like "transition", errors are ignored
tryCatch
calls the function inside a try catch to catch the error and return it to allow the callee to determine what they want to do with the errorChange Type (required)
Indicate the type of change your pull request is:
patch
minor
major
Release Notes
iOSPlayer to PlayerUI Migration Guide
This guide contains the breaking changes between iOSPlayer and PlayerUI
FlowController Transition
FlowControllers
transition
function can throw errors in Core Player, however it's not called as a part of a player process so these errors were ignored on iOS.The change makes the
transition
function a throwing function. Any existing calls to this now require thetry
keywordBefore
After
Error is returned from the
tryCatch
function if it existsUsage After
Usage Before
Version
Published prerelease version:
0.6.0-next.2
Changelog
🚀 Enhancement
🐛 Bug Fix
com.intuit.player:j2v8
transitive deps #256 (@sugarmanz)📝 Documentation
Authors: 9