-
Notifications
You must be signed in to change notification settings - Fork 136
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
Custom text and binary attachments in test apps #179
Custom text and binary attachments in test apps #179
Conversation
Replaced async and await with promise for getErrorAttachments function in Crashes module as they are incompatible with Node 6 |
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.
see my comments
TestApp/CrashesScreen.js
Outdated
@@ -93,14 +105,91 @@ export default class CrashesScreen extends Component { | |||
Crash native code | |||
</Text> | |||
</TouchableOpacity> | |||
<TouchableOpacity onPress={() => {this.dialogComponent.show()}}> | |||
<Text style={styles.button}> | |||
Set text attachmet |
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.
=> "Set text error attachment"
TestApp/CrashesScreen.js
Outdated
<Text style={SharedStyles.enabledText}>{'Current value:'}{this.state.textAttachment}</Text> | ||
<TouchableOpacity onPress={this.showFilePicker}> | ||
<Text style={styles.button}> | ||
Set file attachmet |
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.
=> "Select image as binary error attachment"
TestApp/ios/TestApp/Info.plist
Outdated
<string>$(PRODUCT_NAME) would like to use your camera</string> | ||
<key>NSPhotoLibraryAddUsageDescription</key> | ||
<string>$(PRODUCT_NAME) would like to save photos to your photo gallery</string> | ||
<key>NSMicrophoneUsageDescription</key> |
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.
Is microphone permission necessary?
appcenter-crashes/Crashes.js
Outdated
const attachments = getErrorAttachmentsMethod(report); | ||
AppCenterReactNativeCrashes.sendErrorAttachments(attachments, report.id); | ||
getErrorAttachmentsMethod(report) | ||
.then(attachments => { |
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.
styling issue - you don't need curly brackets for one-line arrow function body. You can do something like this:
.then(attachments => AppCenterReactNativeCrashes.sendErrorAttachments(attachments, report.id))
.catch(error => AppCenterLog.error(LOG_TAG, 'Crashes.getErrorAttachments: ' + error));
appcenter-crashes/Crashes.js
Outdated
AppCenterReactNativeCrashes.sendErrorAttachments(attachments, report.id); | ||
}) | ||
.catch(error => { | ||
AppCenterLog.error(LOG_TAG, 'Crashes.getErrorAttachments: ' + 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.
Please run eslint TestApp/*.js
. I see ~80 eslint errors in the TestApp alone, we should fix or suppress them.
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.
Please follow eslint getting started guide (https://eslint.org/docs/user-guide/getting-started) if you haven't installed eslint globally on your machine.
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 recommend installing ESLint plugin for VS Code if you're using VS Code.
TestApp/ios/Podfile
Outdated
@@ -14,4 +14,8 @@ target 'TestApp' do | |||
pod 'AppCenterReactNativeShared', :path => '../../AppCenterReactNativeShared/Products/' | |||
|
|||
platform :ios, '8.0' | |||
pod 'react-native-image-picker', :path => '../node_modules/react-native-image-picker' |
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.
Because you installed react-native-image-picker
and RNFS
from package.json, hence it's unnecessary to install them from Podfile. You only need one of npm and cocoapod to install your dependencies.
I verified this by removing these two lines and npm install && react-native link
still works as expected.
</View> | ||
); | ||
} | ||
|
||
getTextAttachmentDialog() { |
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 recommend check-in two large image files as part of the TestApp to make testing easier. Due to the error attachment size limit of 1.4 MB on android and 10 MB on ios, I recommend a image < 1.4 MB and another image < 10 MB.
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.
Or create two base64 strings instead of check-in binary image files. We can't assume testing devices have such large binary files for testing.
TestApp/MainScreen.js
Outdated
ErrorAttachmentLog.attachmentWithText('hello', 'hello.txt'), | ||
ErrorAttachmentLog.attachmentWithBinary(testIcon, 'icon.png', 'image/png') | ||
]; | ||
return new Promise(async (resolve, reject) => { |
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.
It is an anti-pattern to use async inside Promise. Because ES2017 async
function returns a Promise and you don't care about reject
here, this code can be simplified with the following:
return (async () => {
const textAttachment = await AttachmentsProvider.getTextAttachment();
const binaryAttachment = await AttachmentsProvider.getBinaryAttachment();
const binaryName = await AttachmentsProvider.getBinaryName();
const binaryType = await AttachmentsProvider.getBinaryType();
return [ErrorAttachmentLog.attachmentWithText(textAttachment, 'hello.txt'),
ErrorAttachmentLog.attachmentWithBinary(binaryAttachment, binaryName, binaryType)];
})();
OR use Promise.all()
to run in parallel:
return (async () => {
const [textAttachment, binaryAttachment, binaryName, binaryType] = await Promise.all([
AttachmentsProvider.getTextAttachment(),
AttachmentsProvider.getBinaryAttachment(),
AttachmentsProvider.getBinaryName(),
AttachmentsProvider.getBinaryType(),
]);
return [ErrorAttachmentLog.attachmentWithText(textAttachment, 'hello.txt'),
ErrorAttachmentLog.attachmentWithBinary(binaryAttachment, binaryName, binaryType)];
})();
TestApp/AttachmentsProvider.js
Outdated
console.log('Binary attachment saved'); | ||
}) | ||
.catch((err) => { | ||
console.log(err.message); |
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.error(err.message)
Addressed feedback and fixed eslint errors |
appcenter-crashes/Crashes.js
Outdated
AppCenterReactNativeCrashes.sendErrorAttachments(attachments, report.id); | ||
getErrorAttachmentsMethod(report) | ||
.then(attachments => AppCenterReactNativeCrashes.sendErrorAttachments(attachments, report.id)) | ||
.catch(error => AppCenterLog.error(LOG_TAG, 'Crashes.getErrorAttachments: ' + 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.
.catch()
will catch any error thrown from the Promise chain. In other words, it can be error from getErrorAttachmentsMethod()
or sendErrorAttachments()
. You need to improve this error message.
@@ -4,6 +4,8 @@ | |||
import android.util.Log; | |||
|
|||
import com.facebook.react.ReactApplication; | |||
import com.rnfs.RNFSPackage; |
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.
styling: sort imports
remoteGlobalIDString = F12AFB9B1ADAF8F800E0535D; | ||
remoteInfo = RNFS; | ||
}; | ||
8037A3C81FD68F570006BF65 /* PBXContainerItemProxy */ = { |
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.
We should remove RNFS-tvOS ios build target as RN SDK doesn't need tvOS now.
@@ -0,0 +1,69 @@ | |||
import { AsyncStorage } from 'react-native'; |
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.
styling: use 2 space indentation instead of 4 space
TestApp/CrashesScreen.js
Outdated
let u = -1; | ||
do { | ||
fileSize /= thresh; | ||
++u; |
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.
styling: indentation
appcenter-crashes/Crashes.js
Outdated
AppCenterReactNativeCrashes.sendErrorAttachments(attachments, report.id); | ||
Promise.resolve(getErrorAttachmentsMethod(report)) | ||
.then(attachments => AppCenterReactNativeCrashes.sendErrorAttachments(attachments, report.id)) | ||
.catch(error => AppCenterLog.error(LOG_TAG, `Crashes.sendErrorAttachments:${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.
.sendErrorAttachments
is not a method of Crashes
class. It is an internal helper method of Helper
class.
I suggest change the log message to Could not send error attachments. Error: ${error}
TestApp/CrashesScreen.js
Outdated
}); | ||
|
||
function getFileName(response) { | ||
return response.fileName != null ? response.fileName : 'binary.jpeg'; |
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.
use !==
instead of !=
.
fileName != null
will false when fileName
is null
or undefined
.
TestApp/CrashesScreen.js
Outdated
} | ||
|
||
function getFileType(response) { | ||
return response.type != null ? response.type : 'image/jpeg'; |
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.
same here, see my other comment.
TestApp/CrashesScreen.js
Outdated
> | ||
<View> | ||
<TextInput | ||
style={{ |
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.
Please refactor all UI styles in this class into SharedStyles.js
so that we keep all StyleSheet in one place.
AttachmentsProvider.saveTextAttachment(this.state.textAttachment); | ||
this.dialogComponent.dismiss(); | ||
}} | ||
> |
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.
[styling]: please use html tag </TouchableOpacity>
instead of >
to be consistent with other JSX files in this repo.
Please do not merge this yet, as there are some changes that need to be discussed.
This PR adds support to set text and binary attachments to test app. There is one major change in Crashes module, I've made getErrorAttachments function async so we could wait for attachments to be read from disk.
This caused this change in Crashes module:
If this solution is ok, I'll change other test apps too.