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

Bump Sentry JavaScript 7 #2250

Merged
merged 20 commits into from
Jun 11, 2022
Merged

Bump Sentry JavaScript 7 #2250

merged 20 commits into from
Jun 11, 2022

Conversation

lucas-zimerman
Copy link
Collaborator

@lucas-zimerman lucas-zimerman commented May 20, 2022

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

  • Bump Sentry JavaScript to version 7.
  • Add support for Mac M1 on the Sample (arm64 simulator)
  • Fix Break changes.
  • Add option to debug the sample app on iOS
  • Merged the backend on the client code.
  • Wrapper now sends envelopes and not events.

💡 Motivation and Context

💚 How did you test it?

For now I just run the sample and waited for an event from the native SDK.

https://sentry.io/organizations/sentry-sdks/discover/sentry-react-native:d1f62b5dbf5c4c8daf8f8bb737e2680f/?field=title&field=event.type&field=project&field=user.display&field=timestamp&name=All+Events&query=&sort=-timestamp&statsPeriod=24h&yAxis=count%28%29

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • All tests passing
  • No breaking changes

🔮 Next steps

  • Fix tests.
  • Fix Break changes that may not be implemented correctly.
  • Refactor how to correctly consume Envelopes to be sent to the Native layer.
  • Fix Scope Sync.
  • Validate if we'll use the flag allowSyntheticDefaultImports
  • Check if the Client/Transport layer are working properly.
  • Fix Sdk version/name on CaptureEnvelope
  • Validate if remaining integrations are working properly.

@lucas-zimerman lucas-zimerman changed the title DRAFTL Bump Sentry JavaScript 7 (Not to merge) DRAFT: Bump Sentry JavaScript 7 (Not to merge) May 20, 2022
@lucas-zimerman
Copy link
Collaborator Author

@marandaneto Should we drop the support of Severity in favor of SeverityLevel, or keep both?

@lucas-zimerman lucas-zimerman changed the title DRAFT: Bump Sentry JavaScript 7 (Not to merge) Bump Sentry JavaScript 7 (Not to merge) May 30, 2022
@lucas-zimerman lucas-zimerman changed the title Bump Sentry JavaScript 7 (Not to merge) Bump Sentry JavaScript 7 May 31, 2022
@marandaneto
Copy link
Contributor

@AbhiPrasad and @lobsterkatie Would you be so kind to do a quick/high-level review in this PR since you all have worked on the JS 7 major release? otherwise, I'd need to learn all the breaking changes just for the review.
Much appreciated.

@marandaneto
Copy link
Contributor

@marandaneto Should we drop the support of Severity in favor of SeverityLevel, or keep both?

This change likely requires a major bump, so I'd rather do what the JS SDK 7.x does.

@AbhiPrasad
Copy link
Member

Please drop support of Severity in favor of SeverityLevel!

cc @getsentry/team-web-sdk-frontend in general, please lets get more eyes on this!

@AbhiPrasad
Copy link
Member

Also - now that the official release has been cut, we now have a proper final CHANGELOG: https://github.com/getsentry/sentry-javascript/releases/tag/7.0.0

src/js/sdk.tsx Outdated Show resolved Hide resolved
@lucas-zimerman lucas-zimerman marked this pull request as ready for review June 6, 2022 16:50
@lucas-zimerman
Copy link
Collaborator Author

lucas-zimerman commented Jun 6, 2022

Android e2e seems broken, but not due to this PR, see a test that ran on the release of 3.4.3: https://github.com/getsentry/sentry-react-native/runs/6703000870?check_suite_focus=true

EDIT: The Code for consuming Envelopes seems broken on Android, I'll give it a look on it.

@marandaneto
Copy link
Contributor

Android e2e seems broken, but not due to this PR, see a test that ran on the release of 3.4.3: getsentry/sentry-react-native/runs/6703000870?check_suite_focus=true

EDIT: The Code for consuming Envelopes seems broken on Android, I'll give it a look on it.

The e2e test is on iOS, it's an issue with the GH action environments, it breaks sometimes and it fixes by itself, it's strange, and never found out the reason.

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

1st pass

src/js/client.ts Outdated Show resolved Hide resolved
src/js/client.ts Show resolved Hide resolved
src/js/integrations/reactnativeerrorhandlers.ts Outdated Show resolved Hide resolved
src/js/sdk.tsx Outdated Show resolved Hide resolved
@lucas-zimerman
Copy link
Collaborator Author

lucas-zimerman commented Jun 7, 2022

Ok I spotted the problem with the Java SDK.

The message payload on the Android side always has been different and I removed the payload code from the envelope.
Turned out the message adapter was still required so I moved it back.

src/js/wrapper.ts Outdated Show resolved Hide resolved
@marandaneto
Copy link
Contributor

@lucas-zimerman left a few comments but LGTM.
Is there something pending or all good? if so, after addressing the comments, we can merge and cut a preview release.

Thanks for doing this.

@lucas-zimerman
Copy link
Collaborator Author

@lucas-zimerman left a few comments but LGTM. Is there something pending or all good? if so, after addressing the comments, we can merge and cut a preview release.

Thanks for doing this.

all good :)

@marandaneto marandaneto merged commit 1e21438 into main Jun 11, 2022
@marandaneto marandaneto deleted the feat/js-sdk-7 branch June 11, 2022 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants