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: disable offline caching on unknown platforms #770

Merged
merged 1 commit into from
May 31, 2022
Merged

Conversation

vaind
Copy link
Collaborator

@vaind vaind commented May 30, 2022

No description provided.

@github-actions
Copy link
Contributor

github-actions bot commented May 30, 2022

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 45fee21

@bruno-garcia
Copy link
Member

Downside is that it could work on xbox/playstation and we'd be turning it off.
I wonder if it's best to just ifdef Switch and turn it off only then

@vaind
Copy link
Collaborator Author

vaind commented May 30, 2022

Downside is that it could work on xbox/playstation and we'd be turning it off. I wonder if it's best to just ifdef Switch and turn it off only then

With this being a handler for "unknown platform", we could white-list the platforms we know/think it would work and leave disable by default. https://docs.unity3d.com/ScriptReference/RuntimePlatform.html

I don't really know which ones to enable, though...

@@ -11,14 +11,23 @@ public static class SentryUnknownPlatform
{
public static void Configure(SentryUnityOptions options)
{
options.DiagnosticLogger?.Log(SentryLevel.Debug,
"Configuring on an unknown platform - some options may be overridden to improve compatibility.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Configuring on an unknown platform - some options may be overridden to improve compatibility.");
"Configuring on an unsupported platform - some options may be overridden to improve compatibility.");

What do you think about unsupported instead of unknown? Because we actually do know the current platform, it's just not supported (yet).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was considering this. Why I chose not to is because it's not really "unsupported", just not "officially supported"

// This is only provided on a best-effort basis for other than the explicitly supported platforms.
if (options.BackgroundWorker is null)
{
options.DiagnosticLogger?.Log(SentryLevel.Debug,
"Configuring on an unknown platform. Using WebBackgroundWorker to improve compatibility.");
options.DiagnosticLogger?.Log(SentryLevel.Debug, "Unknown platform: using WebBackgroundWorker.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
options.DiagnosticLogger?.Log(SentryLevel.Debug, "Unknown platform: using WebBackgroundWorker.");
options.DiagnosticLogger?.Log(SentryLevel.Debug, "Unsupported platform: using WebBackgroundWorker.");


if (options.CacheDirectoryPath is not null)
{
options.DiagnosticLogger?.Log(SentryLevel.Debug, "Unknown platform: disabling offline caching.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
options.DiagnosticLogger?.Log(SentryLevel.Debug, "Unknown platform: disabling offline caching.");
options.DiagnosticLogger?.Log(SentryLevel.Debug, "Unsupported platform: disabling offline caching.");

@@ -11,14 +11,23 @@ public static class SentryUnknownPlatform
{
public static void Configure(SentryUnityOptions options)
{
options.DiagnosticLogger?.Log(SentryLevel.Debug,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
options.DiagnosticLogger?.Log(SentryLevel.Debug,
options.DiagnosticLogger?.Log(SentryLevel.Info,

What do you think about raising this to Info?

Comment on lines 10 to +11
- Use single-threaded HTTP transport on unknown platforms ([#756](https://github.com/getsentry/sentry-unity/pull/756))
- Disable offline caching on unknown platforms ([#770](https://github.com/getsentry/sentry-unity/pull/770))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Use single-threaded HTTP transport on unknown platforms ([#756](https://github.com/getsentry/sentry-unity/pull/756))
- Disable offline caching on unknown platforms ([#770](https://github.com/getsentry/sentry-unity/pull/770))
- Use single-threaded HTTP transport on currently unsupported platforms ([#756](https://github.com/getsentry/sentry-unity/pull/756))
- Disable offline caching on currently unsupported platforms ([#770](https://github.com/getsentry/sentry-unity/pull/770))

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.

The word "Unsupported" like that might give the wrong message? Maybe reword the whole thing to be more clear:

"Platform support for Offline Caching is unknown. Disabling it"

@vaind
Copy link
Collaborator Author

vaind commented May 31, 2022

used @bruno-garcia's suggested wording - that's what I wanted the message to convey.

@vaind vaind merged commit 2366267 into main May 31, 2022
@vaind vaind deleted the feat/disable-cache branch May 31, 2022 12:52
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.

3 participants