-
Notifications
You must be signed in to change notification settings - Fork 6
Conversation
lib/src/functions_client.dart
Outdated
_headers | ||
..clear() | ||
..addAll(Constants.defaultHeaders) | ||
..addAll(authHeader == null ? {} : {'Authorization': authHeader}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove this line to make the behavior a bit more intuitive, but what do you think? If the user wants to remove the auth headers, they can simply omit it from value
.
..addAll(authHeader == null ? {} : {'Authorization': authHeader}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because of ..clear() we need to re-include the auth header after,
maybe an if else is better than ..addAll(authHeader == null ? {} : {'Authorization': authHeader})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@itisnajim
99% of the time the user is using this library from supabase-flutter or supabase-dart, which would have the auth header in value
, so it really doesn't matter, but I feel the inconsistency here. In this case, is the idea to do the same in supabase-dart as well and reserve the auth header?
supabase/supabase-dart#185 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me, it just feels a bit counter intuitive to do this to remove something from the header.
functionsClient.header = {'my-header': 'some-value'};
functionsClient.setAuth(null);
It feels natural if auth header was removed with just this.
functionsClient.header = {'my-header': 'some-value'};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, you are right. I agree with you, so there needs to be the same thing in functionsClient, storageClient
, and realtimeClient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't thought about rls. I don't know realtime good enough for that. Maybe @dshukertjr can help here or can ask the team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to remove realtime channel, and re-initialize realtime-channel to reconnect and send the updated headers.
I feel like we might be over complicating things a bit here.
I guess if someone wants to set a custom headers, they can always just initialize Supabase client with the headers option, but @itisnajim what is the primary reason why this does not solve what you want to achieve? The headers set here will be applied to all client libraries, including realtime, so we don't have to worry anything about reconnecting.
await Supabase.initialize(
url: supabaseUrl,
anonKey: supabaseKey,
headers: {'device_id': '123'},
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to be able to change the headers multiple times and at any time, not only in the construction, things should be dynamic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
As I as said earlier, to update the headers used to access realtime, you can remove channels and re-subscribe again.
I think if someone wants to dynamically modify the headers , they should handle realtime manually by themselves.
To complete the implementation that we have, why don't we just expose a getter for headers
in all of the sub libraries, and introduce a setter and a getter on supabase-dart. Within the method of setter on supabase-dart, we can update each sub libraries' headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @dshukertjr @Vinzent03
we need a merge 😆 of each:
#19
supabase/realtime-dart#63
supabase/storage-dart#57
the client too:
supabase/supabase-dart#185
ca63d22
to
311f548
Compare
What kind of change does this PR introduce?
This PR introduces a feature that allows for greater customization of HTTP headers in the Supabase Dart library. but we need first to enable the customization also in the sub packages (realtime, storage and functions)
Everything is described here:
supabase/supabase-dart#185
What is the current behavior?
Please link any relevant issues here.
What is the new behavior?
Feel free to include screenshots if it includes visual changes.
Additional context
Add any other context or screenshots.