Skip to content
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

feat: Bump sentry-cocoa to 6.1.4 + Native Wrapper and Cordova Transport #194

Merged
merged 15 commits into from
Feb 17, 2021

Conversation

jennmueng
Copy link
Member

@jennmueng jennmueng commented Feb 13, 2021

Notable Changes introduced in this PR

Testing

Added some tests for the wrapper adapted from React Native, however the former integration tests had to be removed as the SDK went through a major overhaul.

Manually tested on a sample app on browser, iOS, and Android.

Next Steps

@codecov-io
Copy link

codecov-io commented Feb 13, 2021

Codecov Report

Merging #194 (e99b2d5) into master (5b634be) will decrease coverage by 40.18%.
The diff coverage is 49.75%.

Impacted file tree graph

@@             Coverage Diff              @@
##            master     #194       +/-   ##
============================================
- Coverage   100.00%   59.81%   -40.19%     
============================================
  Files            1       15       +14     
  Lines            9      321      +312     
  Branches         1       66       +65     
============================================
+ Hits             9      192      +183     
- Misses           0      128      +128     
- Partials         0        1        +1     
Impacted Files Coverage Δ
src/js/scope.ts 40.00% <40.00%> (ø)
src/js/wrapper.ts 40.19% <40.19%> (ø)
src/js/sdk.ts 52.94% <50.00%> (ø)
src/js/transports/cordova.ts 54.16% <54.16%> (ø)
src/js/utils.ts 61.53% <61.53%> (ø)
src/js/backend.ts 78.12% <76.47%> (ø)
src/js/client.ts 90.90% <100.00%> (ø)
src/js/sentry-cordova.ts 100.00% <100.00%> (ø)
src/js/types.ts 100.00% <100.00%> (ø)
src/js/integrations/cordova.ts 50.00% <0.00%> (ø)
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b634be...e99b2d5. Read the comment docs.

@jennmueng jennmueng changed the base branch from master to jenn/bump-js February 16, 2021 17:10
@jennmueng jennmueng marked this pull request as ready for review February 16, 2021 17:15
@jennmueng jennmueng requested review from bruno-garcia and a team February 16, 2021 17:15
@jennmueng jennmueng marked this pull request as draft February 16, 2021 17:16
@jennmueng jennmueng marked this pull request as ready for review February 16, 2021 20:07
@@ -26,6 +26,7 @@
"dependencies": {
"@sentry/browser": "6.1.0",
"@sentry/core": "6.1.0",
"@sentry/hub": "6.1.0",
Copy link
Member

Choose a reason for hiding this comment

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

There's a package just for the Hub now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, and we need to import it to use getCurrentHub

}
return true;
switch(action) {
case "startWithOptions":
Copy link
Member

Choose a reason for hiding this comment

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

We're keeping the iOS name as a standard?
Since sentry-cocoa is the outliner, wouldn't it be better to call it init instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm the name actually comes from the native bridge on RN (I realized that was actually the first thing I was tasked with at Sentry lol getsentry/sentry-react-native#860). So I figured we should keep the naming the same, save for when we do the unified native bridge module for hybrid SDKs.

src/ios/SentryCordova.m Show resolved Hide resolved
src/ios/SentryCordova.m Outdated Show resolved Hide resolved
src/ios/SentryCordova.m Outdated Show resolved Hide resolved
src/ios/SentryCordova.m Outdated Show resolved Hide resolved
- (void)setContext:(CDVInvokedUrlCommand *)command {
[self.commandDelegate runInBackground:^{
NSString *key = [command.arguments objectAtIndex:0];
NSDictionary *context = [command.arguments objectAtIndex:1];
Copy link
Member

Choose a reason for hiding this comment

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

Could be a follow up PR after releasing a preview but shouldn't we use some defensive programming here and check in fact we have 2 items to read from arguments and that they are indeed of the type we assume they are?

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

Good stuff! Left a few notes

@jennmueng jennmueng changed the base branch from jenn/bump-js to master February 17, 2021 13:25
@jennmueng jennmueng merged commit ca52adb into master Feb 17, 2021
@lucas-zimerman lucas-zimerman deleted the jenn/bump-ios branch September 15, 2022 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants