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 Supabase Integration #15719

Open
wants to merge 36 commits into
base: develop
Choose a base branch
from
Open

Conversation

onurtemizkan
Copy link
Collaborator

@onurtemizkan onurtemizkan commented Mar 18, 2025

Ref: #15436

Summary:

Copy link
Contributor

github-actions bot commented Mar 18, 2025

size-limit report 📦

Path Size % Change Change
@sentry/browser 23.2 KB - -
@sentry/browser - with treeshaking flags 23.02 KB - -
@sentry/browser (incl. Tracing) 36.83 KB - -
@sentry/browser (incl. Tracing, Replay) 73.99 KB - -
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 67.39 KB - -
@sentry/browser (incl. Tracing, Replay with Canvas) 78.66 KB - -
@sentry/browser (incl. Tracing, Replay, Feedback) 91.22 KB - -
@sentry/browser (incl. Feedback) 40.33 KB - -
@sentry/browser (incl. sendFeedback) 27.83 KB - -
@sentry/browser (incl. FeedbackAsync) 32.63 KB - -
@sentry/react 25 KB - -
@sentry/react (incl. Tracing) 38.75 KB - -
@sentry/vue 27.41 KB - -
@sentry/vue (incl. Tracing) 38.55 KB - -
@sentry/svelte 23.23 KB - -
CDN Bundle 24.44 KB - -
⛔️ CDN Bundle (incl. Tracing) (max: 38.09 KB) 38.23 KB +3.76% +1.38 KB 🔺
CDN Bundle (incl. Tracing, Replay) 73.21 KB +1.88% +1.34 KB 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) 78.39 KB +1.71% +1.32 KB 🔺
CDN Bundle - uncompressed 71.24 KB - -
CDN Bundle (incl. Tracing) - uncompressed 112.73 KB +3.48% +3.79 KB 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed 224.02 KB +1.73% +3.79 KB 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 236.59 KB +1.63% +3.79 KB 🔺
@sentry/nextjs (client) 40.06 KB - -
@sentry/sveltekit (client) 37.26 KB - -
@sentry/node 142.9 KB - -
@sentry/node - without tracing 96.1 KB - -
@sentry/aws-serverless 120.45 KB - -

View base workflow run

@onurtemizkan onurtemizkan force-pushed the onur/supabase-integration branch 2 times, most recently from dae5b0f to 80d38ce Compare March 18, 2025 12:54
@onurtemizkan onurtemizkan changed the title feat(core): Add Supabase Integration feat Mar 18, 2025
@onurtemizkan onurtemizkan changed the title feat feat: Add Supabase Integration Mar 18, 2025
@onurtemizkan onurtemizkan force-pushed the onur/supabase-integration branch 5 times, most recently from 7d5968b to 73d5f23 Compare March 20, 2025 12:46
@smeubank smeubank linked an issue Mar 20, 2025 that may be closed by this pull request
@onurtemizkan onurtemizkan force-pushed the onur/supabase-integration branch 5 times, most recently from 927f5ab to aaec090 Compare March 26, 2025 13:18
@onurtemizkan onurtemizkan marked this pull request as ready for review March 26, 2025 21:27
@onurtemizkan onurtemizkan force-pushed the onur/supabase-integration branch from dabdf90 to d3a55a1 Compare March 27, 2025 08:57
@onurtemizkan onurtemizkan marked this pull request as draft March 27, 2025 14:14
@onurtemizkan onurtemizkan force-pushed the onur/supabase-integration branch from 5dd6723 to 33c2a66 Compare March 27, 2025 19:58
@smeubank
Copy link
Member

lookin good @onurtemizkan! part of getting this in will be updating the docs, and we can do some changelog post and updates to landing page, but i can help with all that it's simple

@onurtemizkan
Copy link
Collaborator Author

@smeubank thanks, it would be great! We can then merge in queues support separately.

@onurtemizkan onurtemizkan force-pushed the onur/supabase-integration branch from 1bc2897 to d387514 Compare March 31, 2025 10:26
AUTH_ADMIN_OPERATIONS_TO_INSTRUMENT.forEach((operation: AuthAdminOperationName) => {
const authAdminOperation = auth.admin[operation];
if (typeof authAdminOperation === 'function') {
auth.admin[operation] = instrumentAuthOperation(authAdminOperation);
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this need the isAdmin as a second parameter if it is an admin operation? 🤔

Maybe also add a test for the admin behavior.


function instrumentSupabaseAuthClient(supabaseClientInstance: SupabaseClientInstance): void {
const auth = supabaseClientInstance.auth;

Copy link
Member

Choose a reason for hiding this comment

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

Do we need to check for instrumented.has() here as well?

type AuthOperationFn = (...args: unknown[]) => Promise<unknown>;
type AuthOperationName = (typeof AUTH_OPERATIONS_TO_INSTRUMENT)[number];
type AuthAdminOperationName = (typeof AUTH_ADMIN_OPERATIONS_TO_INSTRUMENT)[number];
type PostgrestQueryOperationName = (typeof AVAILABLE_OPERATIONS)[number];
Copy link
Member

Choose a reason for hiding this comment

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

Just to be sure: This is PostgREST and not PostgreSQL?

Super Nit: Call the variable PostgREST.... so it's easier to read and understand. As it was lower-case, I read PostgreSQL at first sight.

Also valid for all other variable names in the file. But ignore this comment if you think this is not necessary.

Copy link
Member

@smeubank smeubank Apr 1, 2025

Choose a reason for hiding this comment

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

const table = pathParts.length > 0 ? pathParts[pathParts.length - 1] : '';
const description = `from(${table})`;

const query: string[] = [];
Copy link
Member

Choose a reason for hiding this comment

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

As it's an array, I would suggest adding items to the name. "Query" kinda suggests it's a string.

Suggested change
const query: string[] = [];
const queryItems: string[] = [];

};
}) satisfies IntegrationFn;

export const supabaseIntegration = defineIntegration((supabaseClient: unknown) => {
Copy link
Member

@s1gr1d s1gr1d Apr 1, 2025

Choose a reason for hiding this comment

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

Should the supabaseIntegration be added by default? If yes, then you should add it to getDefaultIntegrations.

Edit: Ah, as the supabase client needs to be passed, it actually cannot be default 🤔

Copy link
Member

Choose a reason for hiding this comment

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

It's fine to delete this file if it's empty

// These are the default development keys for a local Supabase instance
const NEXT_PUBLIC_SUPABASE_URL = 'http://localhost:54321';
const SUPABASE_SERVICE_ROLE_KEY =
'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJzdXBhYmFzZS1kZW1vIiwicm9sZSI6InNlcnZpY2Vfcm9sZSIsImV4cCI6MTk4MzgxMjk5Nn0.EGIM96RAZx35lJzdJsyH-qQwv8Hdp7fsn3W0YpN81IU';
Copy link
Member

Choose a reason for hiding this comment

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

Is it okay to leave this here? Or should we rather add this to the environment?

Same for NEXT_PUBLIC_SUPABASE_ANON_KEY

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are default local supabase server keys. I think it's ok to leave them here.

onurtemizkan and others added 4 commits April 3, 2025 14:17

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Sigrid Huemer <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@s1gr1d s1gr1d requested a review from lforst April 4, 2025 07:38
Copy link
Member

@lforst lforst left a comment

Choose a reason for hiding this comment

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

A first pass - but it's already looking great!

Comment on lines +488 to +489
export const supabaseIntegration = defineIntegration((supabaseClient: unknown) => {
return {
Copy link
Member

Choose a reason for hiding this comment

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

Different comment here: The client as a simple first arg leaves us a bit unflexible in the future. I would almost make the integration accept an options object with a supabaseClient property.


export const supabaseIntegration = defineIntegration((supabaseClient: unknown) => {
return {
..._supabaseIntegration(supabaseClient),
Copy link
Member

Choose a reason for hiding this comment

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

Can we inline this? I don't fully get the purpose of spreading a function here.

const _supabaseIntegration = (supabaseClient => {
// Instrumenting here instead of `setup` or `setupOnce` because we may need to instrument multiple clients.
// So we don't want the instrumentation is skipped because the integration is already installed.
instrumentSupabase(supabaseClient);
Copy link
Member

Choose a reason for hiding this comment

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

If we potentially need to instrument multiple clients. Would it make sense to either

  • make the integration also accept an array of clients as args
  • maybe export the instrumentSupabase() function so that people can actually call Sentry.init() before they instantiate their supabase clients?


const instrumentSupabase = (supabaseClientInstance: unknown): void => {
if (!supabaseClientInstance) {
throw new Error('Supabase client instance is not available.');
Copy link
Member

Choose a reason for hiding this comment

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

I would probably not throw but log here.

) => Promise<T>;
}

const instrumented = new Map();
Copy link
Member

Choose a reason for hiding this comment

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

I would make this a weakmap or actually a WeakSet should be better for this usecase.

attributes,
});

return (Reflect.apply(target, thisArg, []) as Promise<SupabaseResponse>)
Copy link
Member

Choose a reason for hiding this comment

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

We should always pass the original args. They might be needed in future versions and then we break.

queryItems.push(translateFiltersIntoMethods(key, value));
}

const body: Record<string, unknown> = {};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const body: Record<string, unknown> = {};
const body: Record<string, unknown> = Object.create(null);

to prevent prototype pollution.

@smeubank
Copy link
Member

smeubank commented Apr 9, 2025

@onurtemizkan do you have an example in team-sdks or something for this? I am just curious what this looks like in product, to also look at for the flutter integration later

Adds support for auth and auth.admin operations

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.

Supabase Support
4 participants