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: cleaning the URL properly #136

Merged
merged 6 commits into from
Mar 8, 2024
Merged

fix: cleaning the URL properly #136

merged 6 commits into from
Mar 8, 2024

Conversation

victorforissier
Copy link
Contributor

Doing this pull request because the handling of the access_token on auth breaks user experience. When there are other useful params in the path (e.g. ?docId=xxxxx) the cleanup in the code removes everything instead of just the access_token & refresh_token.

I fixed it by splitting the path and removing just the access_tokens and refresh_tokens and then rebuilding it.

@victorforissier victorforissier requested a review from lowczarc March 8, 2024 06:40
Copy link
Contributor

@lowczarc lowczarc left a comment

Choose a reason for hiding this comment

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

Using filter directly on the splitted object instead of allocating an object and deleting its property to then retransform it into an array would save a few lines and avoid 2 object/array allocations.

@lowczarc
Copy link
Contributor

lowczarc commented Mar 8, 2024

Also, why do you define an anonymous function to call it right after ?

@victorforissier
Copy link
Contributor Author

  1. Yes makes sense. Like that?
  2. Wrapped it inside a function because I think it's clearer and delimits that specific action.

@lowczarc
Copy link
Contributor

lowczarc commented Mar 8, 2024

This is reallocating an anonymous function at runtime every time you're executing it, and it's inconsistent with the rest of the code, pls define your function at global level (and with the function keyword instead of const = () =>...)

@lowczarc
Copy link
Contributor

lowczarc commented Mar 8, 2024

Also the function name is backward

@victorforissier
Copy link
Contributor Author

Your memory tests are broken.

lib/auth.ts Outdated Show resolved Hide resolved
@lowczarc
Copy link
Contributor

lowczarc commented Mar 8, 2024

Your memory tests are broken.

Fixed on main. Don't mind it

@lowczarc lowczarc merged commit 4c5b07e into main Mar 8, 2024
3 checks passed
@lowczarc lowczarc deleted the fix/bug-url-auth-redirect branch March 8, 2024 08:32
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