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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

add Expo config plugin #3391

Merged
merged 5 commits into from
Nov 15, 2023
Merged

Conversation

kbrandwijk
Copy link
Contributor

馃摙 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

馃摐 Description

This PR adds the Expo config plugin that used to be part of the sentry-expo package.
The plugin implementation is identical, except for moving the configuration properties over from the (deprecated) postPublish configuration in app.json to config plugin properties that is the default for config plugins.

馃挕 Motivation and Context

Closes #3263
For more details, see the linked issue and the parent issue.

馃挌 How did you test it?

Tested by building and testing the config plugin locally, and adding it to a local Expo test project to see if the config plugin was applied successfully.

馃摑 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

:shipit:

@krystofwoldrich
Copy link
Member

@kbrandwijk Thank you, this is a good start. There are a few minor things we'll need to adjust.

I've changed the target to expo branch so we can get all changes there before merging to main and releasing.

I would also like to add a simple Expo app to test a build with the plugin in CI. But we can do that in another PR.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@krystofwoldrich
Copy link
Member

We can move the test to /test/expo-plugin/* and change the naming from -test.ts to .test.ts.

We keep all tests in /test.

@krystofwoldrich
Copy link
Member

krystofwoldrich commented Nov 14, 2023

@kbrandwijk So with this PR the configuration of the plugin would look like this:

app.config.js

const { withSentry } = require('@sentry/react-native/plugin/build/withSentry');

const config = {
	name: 'my app',
};

module.exports = withSentry(config, {
	url: 'https://example.sentry.url/',
	authToken: 'example-token',
	project: 'project-slug',
	organization: 'org-slug',
});

OR in JSON

{
	"name": "my app",
	"plugins": [
		[
			"@sentry/react-native/plugin/build/withSentry",
			{
				"url": "https://example.sentry.url/",
				"authToken": "example-token",
				"project": "project-slug",
				"organization": "org-slug"
			}
		]
	]
}

@krystofwoldrich
Copy link
Member

krystofwoldrich commented Nov 14, 2023

To keep the PR small and easy to understand I created some follow-up tasks.

@kbrandwijk
Copy link
Contributor Author

@kbrandwijk So with this PR the configuration of the plugin would look like this:

app.config.js

const { withSentry } = require('@sentry/react-native/plugin/build/withSentry');

const config = {
	name: 'my app',
};

module.exports = withSentry(config, {
	url: 'https://example.sentry.url/',
	authToken: 'example-token',
	project: 'project-slug',
	organization: 'org-slug',
});

OR in JSON

{
	"name": "my app",
	"plugins": [
		[
			"@sentry/react-native/plugin/build/withSentry",
			{
				"url": "https://example.sentry.url/",
				"authToken": "example-token",
				"project": "project-slug",
				"organization": "org-slug"
			}
		]
	]
}

Correct

@kbrandwijk
Copy link
Contributor Author

Thank you for the PR feedback. After applying the lint suggestion, I addressed most of the issues, but was left with a few nasty ones that probably require a bit more work to fix (around the xcode types for example). I disabled those for now, I suggest we circle back to these in a follow up ticket.

@krystofwoldrich
Copy link
Member

@kbrandwijk Sounds good. Thank you.

@kbrandwijk
Copy link
Contributor Author

@kbrandwijk So with this PR the configuration of the plugin would look like this:
app.config.js

const { withSentry } = require('@sentry/react-native/plugin/build/withSentry');

const config = {
	name: 'my app',
};

module.exports = withSentry(config, {
	url: 'https://example.sentry.url/',
	authToken: 'example-token',
	project: 'project-slug',
	organization: 'org-slug',
});

OR in JSON

{
	"name": "my app",
	"plugins": [
		[
			"@sentry/react-native/plugin/build/withSentry",
			{
				"url": "https://example.sentry.url/",
				"authToken": "example-token",
				"project": "project-slug",
				"organization": "org-slug"
			}
		]
	]
}

Correct

Actually, for end user purposes, it would just be:

{
	"name": "my app",
	"plugins": [
		[
			"@sentry/react-native",
			{
				"url": "https://example.sentry.url/",
				"authToken": "example-token",
				"project": "project-slug",
				"organization": "org-slug"
			}
		]
	]
}

@krystofwoldrich krystofwoldrich merged commit 0b80597 into getsentry:expo Nov 15, 2023
23 of 43 checks passed
@krystofwoldrich
Copy link
Member

Thank you. Looks good. 馃憤

@kbrandwijk kbrandwijk deleted the @kbrandwijk/config-plugin branch November 16, 2023 07:14
cfg = withSentryAndroid(cfg, sentryProperties);
} catch (e) {
WarningAggregator.addWarningAndroid(
'sentry-expo',
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like we left some sentry-expo references in here, should update to this lib name

if (!buildGradle.match(pattern)) {
WarningAggregator.addWarningAndroid(
'sentry-expo',
'Could not find react.gradle script in android/app/build.gradle. Please open a bug report at https://github.com/expo/sentry-expo.',
Copy link
Contributor

Choose a reason for hiding this comment

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

we should replace this url with https://github.com/getsentry/sentry-react-native

@objectiveSee
Copy link

Any update on when this release will be stable? We are setting up Sentry for our app that uses Expo 49 and I am having issues with source maps when using Expo's sentry library. If this alpha is almost ready then we might just wait to use this instead.

@krystofwoldrich
Copy link
Member

@objectiveSee We are planning two releases before the stable, which should come in the upcoming weeks.

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.

Migrate Expo Configuration Plugin
4 participants