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: Add setHeader method on postgrest builder #1003

Merged
merged 3 commits into from
Aug 15, 2024
Merged

Conversation

Vinzent03
Copy link
Collaborator

What kind of change does this PR introduce?

feature

What is the current behavior?

To change the headers for one rpc or query call the instance wide headers objects needs to be changed.

What is the new behavior?

After calling .rpc or .from individual header fields can be set with .setHeader(key, value)

Additional context

js-pr: supabase/postgrest-js#550
issue: #179

@Vinzent03 Vinzent03 changed the title feat: setHeader method on postgrest builder feat: SetHeader method on postgrest builder Aug 12, 2024
@Vinzent03 Vinzent03 changed the title feat: SetHeader method on postgrest builder feat: Add setHeader method on postgrest builder Aug 12, 2024
@Vinzent03 Vinzent03 requested a review from dshukertjr August 13, 2024 21:09
@dshukertjr
Copy link
Member

What's the reason why we implemented setHeader method on the query builder and the filter builder instead of having it on the postgrest builder like in JS?

@Vinzent03
Copy link
Collaborator Author

The js implementation is able to return the same type again, as it's not immutable like in Dart. By implementing it only on PostgrestBuilder, the return type would be PostgrestBuilder, which doesn't allow any further filtering after. To be able to call it at every stage in the query building, we would have to implement it in every class. We could do that, but I've found it to be most appealing when called at the start of the query. So I only implemented it for Query- and FilterBuilder, because these are the return types of .from and .rpc.
So I designed it to be .from().setHeader() and .rpc().setHeader().
Though I see that it could be nice to be more free in where to place the setHeader(). So I could implement it in the remaining classes as well.

@dshukertjr
Copy link
Member

Thanks for the detailed explanation! Yeah, if it's not too much trouble, maybe we can add the same method in other classes as well to make it more flexible?

Copy link
Member

@dshukertjr dshukertjr left a comment

Choose a reason for hiding this comment

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

🚀

@Vinzent03 Vinzent03 merged commit efe8e5d into main Aug 15, 2024
8 of 9 checks passed
@Vinzent03 Vinzent03 deleted the feat/set-header branch August 15, 2024 15:00
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