-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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(core): cannot parse action at /session #10094
fix(core): cannot parse action at /session #10094
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
@bwallberg is attempting to deploy a commit to the authjs Team on Vercel. A member of the Team first needs to authorize it. |
Thank you for this fix, @bwallberg! I'm using it via PNPM patch for now. |
@oscarmylla, looking at the error message, I don't this is PR is related. It works without this PR? |
hmm, I'm thinking about using |
Also @bwallberg regarding your comment here, the goal is not to require the That being said, what we're all getting at, I think, is just to expand this fix to the other fraemwork libraries as well (like So long story short, that'd look like this imo:
|
Thanks for clarifying. Sounds very reasonable :) |
I believe I've updated the PR as requested. I've also updated the unit-test to reflect the expectations, which is that the basePath should be set in the config and not in the env variable. I believe setting a custom one only in I've tested the solution in with next.js but not sveltekit, if you approve of the solution I'll setup the local dev app to test it out. |
Awesome, thanks a lot! I'll take a look shortly. Regarding the basePath, yeah it's a bit tricky with SvelteKit because the So long short short, as a user, if i had set |
Yeah I see, that makes sense 🤔 |
@ndom91 I believe I've solved the use-case you've described as well now. |
@bwallberg could you separate the package changes in different PRs - one for core, one for next-auth, and one for sveltekit? just to keep things tidy and the git history clean. Sorry I should've mentioned this earlier. You can keep this PR for core and create new PRs for framework packages. Many thanks! |
@bwallberg thanks, changes in general look good so far! 👍 Was just talking with Thang and thinking about how to avoid foot guns of people misconfiguring this since there are quite a few different permutations that are supported now.. Note that We basically have the following variations we currently support / see used:
Written out, some of those options look like this:
One idea - we should encourage I know many people will probably have something else already configured though because we support / supported many things. Imo to avoid more complexity here, we should pick a "golden path", whatever that looks like, and encourage that everywhere and document it well, and then throw errors / log warnings when folks are setting them to something unsupported. What do you all think? |
I agree to me that felt intuitive that I think it's best to avoid multiple sources of truths that needs to align ( eg. There is a caveat here though that if |
bffb629
to
b7d9640
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10094 +/- ##
==========================================
+ Coverage 39.29% 39.36% +0.06%
==========================================
Files 171 171
Lines 27260 27288 +28
Branches 1154 1165 +11
==========================================
+ Hits 10712 10742 +30
+ Misses 16548 16546 -2 ☔ View full report in Codecov by Sentry. |
@DuarteMartinho I met the similar situation when using 127.0.0.1:3000 to access my website. In my situation, Next always replace |
Hello, I've recently upgraded from In the The first issue I'm having is with the The second issue I'm having is with the |
Have a look at this I made some comments showing my errors and getting it fixed Turns out for me the problem was my next was not updated |
My next version is 14.1.3. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work @bwallberg ! I just push a commit to refactor the warning to a code, using the logger instance. I will also follow up with a doc PR to explain these code. LGTM
* fix(core): cannot parse action at /session fix: support apps hosted on subpaths * refactor(core): use const instead of let for detected host & protocol * fix(core): warn if basePath & AUTH_URL path are both provided * refactor: update logger --------- Co-authored-by: Nico Domino <[email protected]> Co-authored-by: Thang Vu <[email protected]>
* fix(core): cannot parse action at /session fix: support apps hosted on subpaths * refactor(core): use const instead of let for detected host & protocol * fix(core): warn if basePath & AUTH_URL path are both provided * refactor: update logger --------- Co-authored-by: Nico Domino <[email protected]> Co-authored-by: Thang Vu <[email protected]>
☕️ Reasoning
According to the comment for
createActionURL()
it's supposed to build an URL in the same way for both with or withot ENV variables but currently it doesn't use the basePath provided.This causes the
createActionURL()
to create an invalid action url for/session
when one of the ENV variables are set.🧢 Checklist
🎫 Affected issues
Fixes #10081