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

fix: added package name prefix to all console. methods in src/ #699

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

madebyfabian
Copy link

What kind of change does this PR introduce?

After upgrading @supabase/supabase-js, I stumbled on an error message (No storage option exists to persist the session, which may result in unexpected behavior when using auth. If you want to set persistSession to true, please provide a storage option or you may set persistSession to false to disable this warning.).

It took me a while to figure out from which package this comes and why. Found it only because it was reported in the nuxt/supabase repo nuxt-modules/supabase#188.

I therefore suggest to prefix console.error / console.warning with this package's name, so you can easily check where this random error in your console comes from.

What is the current behavior?

Errors are logged into the server console without me having an idea from which package they come from.

What is the new behavior?

Now every error/warning has the package name [@supabase/gotrue-js] prefixed.

Copy link

@sedhha sedhha left a comment

Choose a reason for hiding this comment

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

LGTM

@notjustinshaw
Copy link

Can you point me in the direction of a fix for this (or at least some docs for the persistSession option mentioned). I seem to have this "unexpected behavior" :)

@AdamEisfeld
Copy link

Would love some info on why this warning is happening and how to get rid of it, it is clogging up our supabase logs rendering them useless.

@madebyfabian
Copy link
Author

@AdamEisfeld @notjustinshaw The error appears because some config is not set. Ideally, this would not throw an error, but here we are. The solution in nuxt seems to just set this config value to true/false. But I haven't done any deepdive so it could be something else. nuxt-modules/supabase#188 have you tried that?

@nfts2me
Copy link

nfts2me commented Jun 20, 2023

We are also having this issue.
I guess there is no solution yet, right?

@j4w8n
Copy link
Contributor

j4w8n commented Jun 26, 2023

@nfts2me @notjustinshaw @AdamEisfeld this behavior is designed to help devs be aware that they've got a configuration which could cause them issues. There are plenty of documented cases here of why this was done. Hopefully it's worth it.

Most of the time, you pass the following option to your supabase client - usually just the server-side ones - the error will go away.

const supabase = await createClient(url, key, {
  auth: {
    persistSession: false
  }
}

@nfts2me
Copy link

nfts2me commented Jun 27, 2023

@nfts2me @notjustinshaw @AdamEisfeld this behavior is designed to help devs be aware that they've got a configuration which could cause them issues. There are plenty of documented cases here of why this was done. Hopefully it's worth it.

Most of the time, you pass the following option to your supabase client - usually just the server-side ones - the error will go away.

const supabase = await createClient(url, key, {
  auth: {
    persistSession: false
  }
}

I'm not even using auth. Setting an optional param to mute a warning about something we're not using seems not reasonable.

@j4w8n
Copy link
Contributor

j4w8n commented Jun 27, 2023

I'm not even using auth. Setting an optional param to mute a warning about something we're not using seems not reasonable.

Good point there!

@notjustinshaw
Copy link

notjustinshaw commented Jun 27, 2023 via email

@madebyfabian
Copy link
Author

Hi there, is there still interest in improving the log messages (which this PR was originally intended to be)? Or can I close this and it's fine due to the core issue being fixed.

@AlexIsMaking
Copy link

Hi there, is there still interest in improving the log messages (which this PR was originally intended to be)? Or can I close this and it's fine due to the core issue being fixed.

IMO this change is still worthwhile, it will make it easier to spot the source of the error before making the changes which are mentioned in the thread to avoid it.

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.

7 participants