Skip to content

Update jwt otp deta - #46

Open
ashishjullia wants to merge 10 commits into
portunus-dev:mainfrom
ashishjullia:update-jwt-otp-deta
Open

Update jwt otp deta#46
ashishjullia wants to merge 10 commits into
portunus-dev:mainfrom
ashishjullia:update-jwt-otp-deta

Conversation

@ashishjullia

@ashishjullia ashishjullia commented Jul 15, 2023

Copy link
Copy Markdown
Contributor

Changes:

Do note:

  • This targets wrangler 3.x
  • Supports latest cf workers

Comment thread handlers.js Outdated
const jwt = require('jsonwebtoken')
const { totp } = require('otplib')
const { v4: uuidv4 } = require('uuid')
import jwt from '@tsndr/cloudflare-worker-jwt'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

perhaps keep the import style consistent (others are require in this file)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Comment thread modules/otp.js

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This file's indentation seems to be inconsistent with others (4-space instead 2). Perhaps run a prettier/lint to auto format

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@woozyking done

@woozyking woozyking left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you @ashishjullia. Looks good!

Will hold on to merging as the wrangler config and CD actions aren't updated yet

@ashishjullia

Copy link
Copy Markdown
Contributor Author

Thank you @ashishjullia. Looks good!

Will hold on to merging as the wrangler config and CD actions aren't updated yet

Oh, if you say, I can create a PR for that as well as I already have those in place, lmk.

@woozyking

Copy link
Copy Markdown
Member

Thank you @ashishjullia. Looks good!

Will hold on to merging as the wrangler config and CD actions aren't updated yet

Oh, if you say, I can create a PR for that as well as I already have those in place, lmk.

That'd be sweet!

@ashishjullia

Copy link
Copy Markdown
Contributor Author

Thank you @ashishjullia. Looks good!
Will hold on to merging as the wrangler config and CD actions aren't updated yet

Oh, if you say, I can create a PR for that as well as I already have those in place, lmk.

That'd be sweet!

You can take a look, here: #47

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