Skip to content
This repository has been archived by the owner on Feb 2, 2023. It is now read-only.

[ASDisplayNode] Add support for setting non-fatal error block #2993

Merged
merged 6 commits into from
Feb 8, 2017

Conversation

rmls
Copy link
Contributor

@rmls rmls commented Feb 6, 2017

Currently Async Display Kit doesn't provide a mechanism for "asserting" in production. In many the framework can recover from failures, but that would happen silently when in production.

This code changes gives the ability to set a "non-fatal error block" on ASDisplayNode class, so it can be plugged in many areas where we have asserts, for example, so developers have the change to capture and report such failures in production.

This addresses #2807 .

Copy link
Contributor

@garrettmoon garrettmoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make a new macro which is called ASNonFatalAssert? In which case it would need to create an error from the assert and call this automatically?

@@ -53,6 +53,12 @@ typedef void (^ASDisplayNodeContextModifier)(_Nonnull CGContextRef context);
typedef ASLayoutSpec * _Nonnull(^ASLayoutSpecBlock)(__kindof ASDisplayNode * _Nonnull node, ASSizeRange constrainedSize);

/**
* AsyncDisplayKit non-faltal error block. This block can be used for handling non-fatal errors. Useful for reporting
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "non-faltal"

@@ -256,6 +262,15 @@ NS_ASSUME_NONNULL_BEGIN
*/
@property (readonly) ASInterfaceState interfaceState;

/**
* @abstract Class property that allows to set a block that can be called on non-fatal errors. This
* property can be useful for cases when Async Display Kit can recover from an anormal behavior, but
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: "anormal"

/**
* @abstract Class property that allows to set a block that can be called on non-fatal errors. This
* property can be useful for cases when Async Display Kit can recover from an anormal behavior, but
* still gives the opportunity to use a reporting mechanism to catch occurrences in production.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add: "In development, ASDK will assert instead of calling this."

@Adlai-Holler
Copy link
Contributor

Jenkins, add to whitelist

Base/ASAssert.h Outdated
@@ -59,3 +59,17 @@
#define ASDisplayNodeCAssertPositiveReal(description, num) ASDisplayNodeCAssert(num >= 0 && num <= CGFLOAT_MAX, @"%@ must be a real positive integer.", description)
#define ASDisplayNodeCAssertInfOrPositiveReal(description, num) ASDisplayNodeCAssert(isinf(num) || (num >= 0 && num <= CGFLOAT_MAX), @"%@ must be infinite or a real positive integer.", description)

#define ASDisplayNodeErrorDomain @"ASDisplayNodeErrorDomain"
#define ASDisplayNodeNonFatalErrorCode 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Total nit, but maybe make this 1? Many systems use zero as a non-error.

Base/ASAssert.h Outdated
#define ASDisplayNodeNonFatalErrorCode 0

#define ASDisplayNodeAssertNonFatal(desc, ...) \
ASDisplayNodeAssert(NO, desc, ##__VA_ARGS__); \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking more along the lines of this above

#define ASDisplayNodeAssertNonFatal(condition, desc, ...)
ASDisplayNodeAssert(condition, desc, ##__VA_ARGS__);
if (condition) {

Copy link
Contributor

@garrettmoon garrettmoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for putting this up Rocir!

@garrettmoon garrettmoon merged commit b33cced into facebookarchive:master Feb 8, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants