Refactor exceptions, code cleanup, and move to delegate auth state changes
#57
Conversation
Remove unused imports. Add missing docs. Minor tweaks.
Remove unused imports. Add missing docs. Minor tweaks.
A few of these imports can cause problems (e.g. Linq, compiler services)
|
Ok, I think I got everything. Take a look, LMK if you have any Qs. FWIW I tried flipping the persistence stuff to use an interface, but delegates are not methods but instance variables and so the interface wouldn't work unless I flipped them to methods (adding more indirection complexity) and/or an abstract class (which wound up just being three methods and a constructor anyways). The way it works now still works nicely with automatically generating types in the IDE and it's all typesafe, so I think it's fine ¯_(ツ)_/¯ If you think it looks good I think my next step is testing it out in Unity. I'm hoping to release a demo Unity project that will make it dead simple for someone to just copy and paste their Supbase config into Unity, hit a validate button in the Editor that will ping the new settings, and they'll be off to the races. Hopefully that will be a very short, easy video too. Then I'll add a second video showing the same thing but with Sign in with Apple. It's a real pity so much of this stuff is such a PITA to test. Oh well. |
|
Ok, now that I'm using the full Supabase I see how the options interact a bit. Minor tweaks to make that easier to work with. Few other minor things: I think that an abstract class for the persistence class works well. Also adding debug implementations of the various handlers that just write to console to make it easier for folks getting started. Hoping to do these tonight/tomorrow. |
|
Okay - I've thought about the delegate changes you proposed all day and I can get behind it! I'll just have to transfer these paradigm changes to the other repos before doing a release upstream on supabase-csharp as these are going to warrant a semver major release (realtime-csharp has needed some love for a little while anyways and the delegates ought to clean up a lot of the code there!) Good! |
acupofjose
added
enhancement
acupofjose
changed the title
delegate auth state changes
|
lol you were totally right RE: the persistence interface. Was totally overthinking/overcomplicating it w/delegates, etc. It's now just a simple interface. |
|
Ok, last tweak for now. I moved setting the persistence from the client options to a method. This allows me to set the persistence without relying on the parent Supabase libs to tweak the options there for testing. |
|
Cool, anything you need me to do at this point? I think the main thing I'd like to do next is beat on it a bit more pointing at a live project to stress a bunch of edge cases. In particular, to make sure the flow works nicely for things that aren't covered by the docker stuff, like the flow for users if auto-confirm is turned off. I also think it would be good to add in some more errors to cover stuff like incorrectly configured nonce values coming back. Mostly I'm hoping those will just be adding more scenarios to the reasons file in the exception. IMHO those can be another PR if you want to close this one, they shouldn't break contracts just add some more hints for when things go sideways. |
|
Nope - thanks so much. You've been a tremendous help! This PR is now available in v4.0.0. Looking forward to any other contributions you wish to make! |
What kind of change does this PR introduce?
From a high level, this PR changes the following:
Sorry for a lot of changes all at once, hopefully this isn't a giant PITA to review.
I think the most "controversial" thing is tweaking the persistence functions to no longer be async. I decided to remove that as the data involved is tiny, the main thread calling refresh is already running on another thread, and it makes it a lot easier to write the persistence plugin. Could go either way on this one.
I did run all the test suite against the local docker setup. :)