Skip to content

Conversation

@Armillus
Copy link
Contributor

@Armillus Armillus commented Jun 16, 2024

πŸ”— Linked issue

#761

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

The linked issue just covered half of the problem ; in the current form, getSession cannot be disabled, even with a configuration extracted from the documentation such as:

{
  auth: {
    baseURL: '/api/auth',
    provider: {
      type: 'local',
      endpoints: {
        getSession: false,
      }
    }
  }
}

It's not an issue while you're not calling it, except that it is called by signIn anyways, at least with the local and refresh providers. Eventually, it means that an unexpected HTTP request is sent, which on one hand is inefficient, and on the other hand throws an exception from signIn, thus making it fail.

To fix those problems, this PR is applying the same strategy for getSession as for signOut, in both the function implementation and the exported type.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have added tests (if possible).
  • I have updated the documentation accordingly.

@Armillus Armillus changed the title fix: disabling getSession for real fix: allowing getSession to be disabled for real Jun 16, 2024
@zoey-kaiser
Copy link
Member

zoey-kaiser commented Jun 27, 2024

Hi @Armillus πŸ‘‹

Firstly, I love the PR title πŸ˜†. Thank you for pushing this change, and I apologize for the short delay. I had just run the CI checks on your PR and sadly there are some prepack issues. Could you please look into resolving these πŸ€—

@zoey-kaiser zoey-kaiser added p3 Minor issue bug A bug that needs to be resolved labels Jun 27, 2024
@Armillus
Copy link
Contributor Author

Hi @zoey-kaiser,

I'm glad you liked the title πŸ˜„ No problem regarding the delay!
The prepack issue should be fixed now.

@zoey-kaiser
Copy link
Member

The prepack issue should be fixed now.

They are indeed! Thank you so much! There is one small linting issue and otherwise I think we are good to go. Once we get this PR through, I will release a first RC of 0.8.0!

@Armillus
Copy link
Contributor Author

Armillus commented Jun 28, 2024

Wonderful! I fixed linting issues as well, sorry for the slight inconvenience.

@zoey-kaiser zoey-kaiser changed the title fix: allowing getSession to be disabled for real fix: properly disable getSession endpoint for local and refresh Jun 28, 2024
@zoey-kaiser zoey-kaiser merged commit e711e39 into sidebase:main Jun 28, 2024
zoey-kaiser added a commit that referenced this pull request Jul 4, 2024
@zoey-kaiser
Copy link
Member

Hi @Armillus πŸ‘‹

After testing this PR in 0.8.0, we decided to revert it. You can find a full explanation here: #781 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug A bug that needs to be resolved p3 Minor issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants