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

Minor Unity Related Fixes #70

Merged
merged 4 commits into from
Jun 28, 2023

Conversation

wiverson
Copy link
Collaborator

@wiverson wiverson commented Jun 26, 2023

What kind of change does this PR introduce?

  1. The Internal enum value is causing an error in Unity "Error CS0118 : 'Internal' is a namespace but is used like a variable". PR renames it to a semantically identical name that's less likely to cause a conflict.
  2. GetAssemblyVersion is used to set X-Client-Info for the requests if the header is not already set. This call to GetAssemblyVersion fails on Unity with a null reference. Replaced this with a Guid set for the app run - this will likely be more helpful for debugging (e.g. tracking down a choreography of requests in a logging solution).

@wiverson wiverson changed the title Change name of enum away from Internal Minor Unity Related Fixes Jun 26, 2023
@acupofjose
Copy link
Collaborator

Odd that GetAssemblyInfo fails in Unity - but the expectation is that the X-Client-Info header conforms to a particular schema (see here: https://github.com/search?q=org%3Asupabase-community+x-client-info&type=code)

You ought to be able to override it in the Headers when initializing a client:

_instance = new Supabase.Client(URL, PUBLIC_KEY, new Supabase.SupabaseOptions
{
  Headers = new Dictionary<string, string>
  {
    ["X-Client-Info"] = Guid.NewGuid().ToString()
  }
});

@wiverson
Copy link
Collaborator Author

wiverson commented Jun 27, 2023

I think GetAssemblyInfo fails for two potential reasons - first, when it's running in the Editor I think it's doing something interesting (bytecode off disk, caching in memory, etc). Second, when it's run through the ILL2CPP compiler it's all just native code and so technically there is no .NET assembly. This might be exacerbated by the fact that I'm using a degit version of the source in my project for testing/dev.

The schema is a bit loose, lol.

I just updated the commit to match closer to the other versions of the code (the specific GoTrue client info + assembly version) by default, while also supporting the Unity scenarios. I'm hoping to have the getting started / out-of-the-box scenario for a Unity dev to be as cake as possible, so either way I'd like a solution that "just works."

If you don't like the tracking session per app launch thing, the version that would match closer semantically would probably be something like:

if (!headers.ContainsKey("X-Client-Info"))
{
	try
	{
		// Default version to match other clients
		// https://github.com/search?q=org%3Asupabase-community+x-client-info&type=code
		headers.Add("X-Client-Info", $"postgrest-csharp/{Util.GetAssemblyVersion(typeof(Client))}");
	}
	catch (Exception)
	{
		// Fallback for when the version can't be found
		// e.g. running in the Unity Editor, ILL2CPP builds, etc.
		headers.Add("X-Client-Info", $"postgrest-csharp/0.0.1");
	}
}

...which looks like the express-js client version. Or perhaps just postgrest-csharp/unknown.

Then just doc the GUID insertion version instead. Just LMK - I prefer the out-of-box GUID for local debug but it's not that big of a deal.

@acupofjose
Copy link
Collaborator

I can dig the App launch thing as a fallback - I'll need to go implement that on the other child repos! Personally I'd place the blame on the ILL2CPP compiler. But regardless, that's a fix that works for me!

As always, thanks for your time @wiverson, it's much appreciated!

@acupofjose acupofjose merged commit 1bb5196 into supabase-community:master Jun 28, 2023
1 check passed
@wiverson wiverson deleted the change-from-internal branch June 28, 2023 15:13
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