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(expo): Added ability to provide sentryUrl for source maps upload #3664

Closed
wants to merge 6 commits into from

Conversation

Acetyld
Copy link
Contributor

@Acetyld Acetyld commented Mar 6, 2024

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Added sentryUrl for people that are self hosting

💡 Motivation and Context

Its also possible to define URL inside app.json and this was missing from the command.

💚 How did you test it?

I tested it locally.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • All tests passing
  • No breaking changes

🔮 Next steps

Comment on lines 152 to 158
if (pluginConfig.url) {
sentryUrl = pluginConfig.url;
} else {
sentryUrl
}
console.log(`${SENTRY_PROJECT} resolved to ${sentryProject} from expo config.`);

Copy link
Member

Choose a reason for hiding this comment

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

Although, I agree with loading the URL from the Expo Plugin Configuration.

The current code won't work, as pluginConfig variable is not in the scope.

This should be under if (!sentryOrg || !sentryProject) {, which needs to be updated to include sentryUrl.

@krystofwoldrich
Copy link
Member

krystofwoldrich commented Mar 7, 2024

@Acetyld Hi, thank you for the PR,

this makes sense and is a good addition to the Expo script.

But before we can add it the code needs a few fixes that I described in the comments above.

@Acetyld
Copy link
Contributor Author

Acetyld commented Mar 8, 2024

Hi =) Yhea i used the browser vscode editor, it was a bit wonky.
Sry for committing above unrelated changed ^^

I saw u fixed almost all issue's so i suppose it is fine now?

@krystofwoldrich
Copy link
Member

@Acetyld There is just this one comment to be resolved.

@Acetyld
Copy link
Contributor Author

Acetyld commented Mar 8, 2024

Alright i tried to update above, if it still incorrect i will put time into doing this in a real editor instead of the git edit 😜

Copy link
Member

@krystofwoldrich krystofwoldrich left a comment

Choose a reason for hiding this comment

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

@Acetyld Thank you, now it's perfect. 🚀

@krystofwoldrich krystofwoldrich changed the title Added ability to provide sentryUrl feat(expo): Added ability to provide sentryUrl for source maps upload Mar 14, 2024
@krystofwoldrich
Copy link
Member

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.

2 participants