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

Added export for isNew, isDestroyed and isTouched from symbol.js #365

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

AcidBurnHen
Copy link

@AcidBurnHen AcidBurnHen commented May 21, 2022

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.

import prisma from "@/lib/prisma";
import { getSession } from "./get-session";
import { isNew } from "next-session/lib/symbol";

const getUser = async (req, res) => {
  /*If the user logs in but we don't get the session then we can't check for user info, but if the
  user does not log in, this will throw a new session. */
  const session = await getSession(req, res);

  /* I only want to fetch a user if the session isn't new, meaning that the
  user already logged in, and interacted with the api after his session started */
  if (!session[isNew]) {
    const user = await prisma.user.findUnique({
      where: {
        id: req.session.user.id
      }
    });

    return user;
  } else {
    return res.end;
  }
};

export const userAllowed = handle => async (req, res) => {
  const user = await getUser(req, res)
    .then(res => {
      return res;
    })
    .catch(err => {
      console.log(err);
    });

  if (user && user.role === "USER") {
    return await handle(req, res);
  } else {
    res.status(401).json({ error: "You cannot access this page, please log in or register" });
  }
};

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 the isDestroyed 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 use session.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.

@changeset-bot
Copy link

changeset-bot bot commented May 21, 2022

⚠️ No Changeset found

Latest commit: 78c3e8e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@codecov
Copy link

codecov bot commented May 21, 2022

Codecov Report

Merging #365 (019c559) into master (862498a) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head 019c559 differs from pull request most recent head 78c3e8e. Consider uploading reports for the commit 78c3e8e to get more accurate results

@@            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              
Impacted Files Coverage Δ
src/helpers.ts 100.00% <100.00%> (ø)
src/session.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 862498a...78c3e8e. Read the comment docs.

@hoangvvo
Copy link
Owner

hoangvvo commented May 23, 2022

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.

@AcidBurnHen
Copy link
Author

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.

@hoangvvo
Copy link
Owner

hoangvvo commented May 24, 2022

Yeah I would prefer we not use the symbols directly. Perhaps we can define some public functions like isNew(), isTouched() and isDestroy() that would use those symbols internally. Usage would be like:

const new = isNew(req.session);

Eg, isNew would be implemeted like:

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.

@AcidBurnHen
Copy link
Author

In my local copy of next-session I made helpers.js as you recommended.

import { isNew as isNewSymbol, isTouched as isTouchedSymbol, isDestroyed as isDestroyedSymbol } from "./symbol.js";
export function isNew(session) {
  return session[isNewSymbol];
}
export function isTouched(session) {
  return session[isTouchedSymbol];
}
export function isDestroyed(session) {
  return session[isDestroyedSymbol];
}

I tested this out with Postman and it works just like before

import { isNew } from "next-session/lib/helpers";

const getUser = async (req, res) => {
  const session = await getSession(req, res);

  console.log(isNew(req.session));

  if (isNew(req.session) === false) {
    const user = await prisma.user.findUnique({
      where: {
        id: req.session.user.id
      }
    });
  } else {
    res.status(405).json({ error: "You need to log in to view this page" });
  }

  return user;
};

I wanted to compile this but I ran into some issues with babel

Cannot find type definition file for 'babel__generator'.
  The file is in the program because:
    Entry point for implicit type library 'babel__generator'

For some reason, I also couldn't compile commonjs files

'BUILD_MODULE' is not recognized as an internal or external command,
operable program or batch file

I can look into these unless you already know what's causing them.

@hoangvvo
Copy link
Owner

Can you push the code and enable editing? I can take a look at it and work on that

@AcidBurnHen
Copy link
Author

AcidBurnHen commented May 25, 2022

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.

"message": "Type '(...args: any) => void' is not assignable to type '{ (cb?: (() => void) | undefined): ServerResponse; (chunk: any, cb?: (() => void) | undefined): ServerResponse; (chunk: any, encoding: BufferEncoding, cb?: (() => void) | undefined): ServerResponse; }'.\n Type 'void' is not assignable to type 'ServerResponse'.",

The error is being generated here

 res.end = function resEndProxy(...args: any) {
        const done = () => _end.apply(this, args);
        if (session[isDestroyed]) {
          done();
        } else if (hash(session) !== prevHash) {
          store.set(session.id, session).finally(done);
        } else if (session[isTouched] && store.touch) {
          store.touch(session.id, session).finally(done);
        } else {
          done();
        }
      };

I tried some things but most didn't work, the problem is when res.end is being set to the resEndProxy function.

I tried

res.end = function resEndProxy(...args: any): ServerResponse

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

res.end = function resEndProxy(...args: any): any

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.

Copy link
Owner

@hoangvvo hoangvvo left a 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";
Copy link
Owner

@hoangvvo hoangvvo Jun 10, 2022

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 @@
{
Copy link
Owner

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": {
Copy link
Owner

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';

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.

None yet

2 participants