-
Notifications
You must be signed in to change notification settings - Fork 24
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
Added export for isNew, isDestroyed and isTouched from symbol.js #365
base: master
Are you sure you want to change the base?
Conversation
|
Codecov Report
@@ Coverage Diff @@
## master #365 +/- ##
==========================================
+ Coverage 99.17% 99.21% +0.03%
==========================================
Files 6 7 +1
Lines 121 127 +6
Branches 37 37
==========================================
+ Hits 120 126 +6
Misses 1 1
Continue to review full report at Codecov.
|
This is a valid use case. However, these symbols are internal to me and are only used to perform specific logic. I am just afraid I might make breaking changes to it. I wonder if it makes sense to export them. |
If you are worried about breaking changes to it, then perhaps if we somehow only exported the boolean value that is the result of the logic in next-session to be used and not the symbols themselves that would remove your worries and we'd still be able to use this logic for our custom needs. |
Yeah I would prefer we not use the symbols directly. Perhaps we can define some public functions like const new = isNew(req.session); Eg, import { isNew as isNewSymbol } from "./symbol";
export function isNew(session) {
return session[isNewSymbol];
} This way, if I am to change some internals, I can just change the logic of this business to use the new ones. |
In my local copy of next-session I made helpers.js as you recommended.
I tested this out with Postman and it works just like before
I wanted to compile this but I ran into some issues with babel
For some reason, I also couldn't compile commonjs files
I can look into these unless you already know what's causing them. |
Can you push the code and enable editing? I can take a look at it and work on that |
I pushed the code and editing is already enabled from what I can see. I accidentally used npm out of habbit so, after re-installing with yarn, i tried running build again, but then got this error.
The error is being generated here
I tried some things but most didn't work, the problem is when res.end is being set to the resEndProxy function. I tried
But then we need to return something to the function, which I assume is meant to stay as void and not declared. In the end using this did the trick
However, I am not that familiar with typescript, I actually started reading up on it yesterday when I needed it for this. So it would definitely be best if you check up on this error and if there is perhaps a better solution. |
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.
So sorry for the late response. I added some comment and updated the test cases.
@@ -0,0 +1,51 @@ | |||
import { isDestroyed, isNew, isTouched } from "../src/helpers"; |
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.
Update the test case to reflect its use cases. There are some failing ones, could you try fixing the helper functions so they can pass?
@@ -1,6 +0,0 @@ | |||
{ |
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.
Why was this file removed? Could you restore it?
@@ -18,6 +18,10 @@ | |||
"./lib/compat": { | |||
"import": "./lib/compat.js", | |||
"require": "./lib/compat.cjs" | |||
}, | |||
"./lib/helpers": { |
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.
We don't need this. You can reexport all the helpers function in src/session.ts
so we can access it like:
import { isNew, isTouched, isDestroyed } from 'next-session';
When building custom backends with user authentication we can use the already built-in feature isNew to additionally protect routes that get accessed after the user is logged in. I am using this in my project for user permissions.
This is just one example of using isNew, we can also get creative with isTouched and isDestroyed to add more custom functionalities to our apps. For example, if we try to make signout and use
session.destroy()
then we might have a need to manipulate our app on theisDestroyed
property to check if the session was properly destroyed and redirect the user.isTouched
also gives us an additional level of creativity when we want to usesession.commit()
instead of autoCommit option. Since all of this logic is already built into next-session, it would be a shame not to use it.