Skip to content
This repository was archived by the owner on Jan 23, 2026. It is now read-only.

fix: limit proxy session warning to once per client instance#900

Merged
kangmingtay merged 2 commits into
supabase:masterfrom
j4w8n:limit-get-session-warning
May 9, 2024
Merged

fix: limit proxy session warning to once per client instance#900
kangmingtay merged 2 commits into
supabase:masterfrom
j4w8n:limit-get-session-warning

Conversation

@j4w8n

@j4w8n j4w8n commented May 6, 2024

Copy link
Copy Markdown
Contributor

What kind of change does this PR introduce?

Bug fix.

What is the current behavior?

A call to getSession(), when using server storage, logs a warning every time session.user is accessed. This is causing a lot of people to see multiple consecutive logs to the console - especially for SvelteKit users.

What is the new behavior?

Ensures that accessing session.user, from a getSession() call, only logs the warning once per Proxy session instance. In other words, once per server request.

Additional context

supabase/supabase-js#1709

@Zanzofily

Zanzofily commented May 6, 2024

Copy link
Copy Markdown

I believe you are just updating the local variable here, which won't be picked up on next invocations.

@j4w8n

j4w8n commented May 6, 2024

Copy link
Copy Markdown
Contributor Author

I believe you are just updating the local variable here, which won't be picked up on next invocations.

Perhaps I got it wrong, but it passes the addition to the test. That let creates a reference to this.suppress... on the client; so it should be changing the value of this.supress... when suppressWarning is changed. Then, when the value is checked again, only for the current server client, it should be true and not emit any further warnings - again, only for the current client.

This is only a per-request "fix".

@Zanzofily

Zanzofily commented May 6, 2024

Copy link
Copy Markdown

I believe you are just updating the local variable here, which won't be picked up on next invocations.

Perhaps I got it wrong, but it passes the addition to the test. That let creates a reference to this.suppress... on the client; so it should be changing the value of this.supress... when suppressWarning is changed. Then, when the value is checked again, only for the current server client, it should be true and not emit any further warnings - again, only for the current client.

This is only a per-request "fix".

References only work for objects in javascript, It's passing the test because you're using the value of session?.user rather than re-calling getSession() inside your test

@j4w8n

j4w8n commented May 6, 2024

Copy link
Copy Markdown
Contributor Author

I believe you are just updating the local variable here, which won't be picked up on next invocations.

Perhaps I got it wrong, but it passes the addition to the test. That let creates a reference to this.suppress... on the client; so it should be changing the value of this.supress... when suppressWarning is changed. Then, when the value is checked again, only for the current server client, it should be true and not emit any further warnings - again, only for the current client.

This is only a per-request "fix".

References only work for objects in javascript, It's passing the test because you're using the value of session?.user rather than re-calling getSession() inside your test

Ah thank you! I'll have to play with it a bit more then.

So, if I understand you, this is only a partial fix: it works only when you re-access session.user, but if you were to call getSession again, then it would log again.

@kangmingtay

kangmingtay commented May 7, 2024

Copy link
Copy Markdown
Member

So, if I understand you, this is only a partial fix: it works only when you re-access session.user, but if you were to call getSession again, then it would log again.

yup that's because a new proxy is created everytime getSession is called and returns the non-expired session

@j4w8n

j4w8n commented May 7, 2024

Copy link
Copy Markdown
Contributor Author

Thanks @kangmingtay and @Zanzofily. I've made a couple of tweaks that I believe resolves the issue with subsequent proxy instance creations.

@kangmingtay, I'm not sure if the team approves of me expanding an existing test. I can separate if needed.

@j4w8n j4w8n changed the title fix: limit proxy session warning to once per instance fix: limit proxy session warning to once per client instance May 7, 2024
@kangmingtay

kangmingtay commented May 9, 2024

Copy link
Copy Markdown
Member

@j4w8n that's fine, thanks for helping with this!

@kangmingtay kangmingtay merged commit 4ecfdda into supabase:master May 9, 2024
@github-actions github-actions Bot mentioned this pull request May 9, 2024
Merged
hf pushed a commit that referenced this pull request Jun 17, 2024
@github-actions
chore(master): release 2.64.3 (#903)
f131300 
🤖 I have created a release *beep* *boop*
---


##
[2.64.3](v2.64.2...v2.64.3)
(2024-06-17)


### Bug Fixes

* don't call removeSession prematurely
([#915](#915))
([e0dc518](e0dc518))
* limit proxy session warning to once per client instance
([#900](#900))
([4ecfdda](4ecfdda))
* patch release workflow
([#922](#922))
([f84fb50](f84fb50))
* type errors in verifyOtp
([#918](#918))
([dcd0b9b](dcd0b9b))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
mandarini pushed a commit to supabase/supabase-js that referenced this pull request Oct 2, 2025
@github-actions
chore(master): release 2.64.3 (#903)
c7b8228 
🤖 I have created a release *beep* *boop*
---


##
[2.64.3](supabase/auth-js@v2.64.2...v2.64.3)
(2024-06-17)


### Bug Fixes

* don't call removeSession prematurely
([#915](supabase/auth-js#915))
([152c40a](supabase/auth-js@152c40a))
* limit proxy session warning to once per client instance
([#900](supabase/auth-js#900))
([9d450f3](supabase/auth-js@9d450f3))
* patch release workflow
([#922](supabase/auth-js#922))
([9efd597](supabase/auth-js@9efd597))
* type errors in verifyOtp
([#918](supabase/auth-js#918))
([9333704](supabase/auth-js@9333704))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@kangmingtay kangmingtay kangmingtay approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@j4w8n @Zanzofily @kangmingtay