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

[ Fix ] Node v20 - Migrate to base32Encode. #70

Merged
merged 2 commits into from
Aug 29, 2024
Merged

Conversation

kldeb
Copy link
Contributor

@kldeb kldeb commented Aug 28, 2024

Solves node v20 issue running on aws lambda

@dev-xo
Copy link
Owner

dev-xo commented Aug 28, 2024

Thanks for the PR, @kldeb.

I noticed some indentation we do not have in the project. Not sure if you can run npm run format in order to fix that. Also, hopefully, @mw10013 could also take a look at the PR too.

@dev-xo dev-xo changed the title migrate to base32Encode [ Fix ] Node v20 - Migrate to base32Encode. Aug 28, 2024
@kldeb
Copy link
Contributor Author

kldeb commented Aug 28, 2024

strange, i did run the format script.

@dev-xo
Copy link
Owner

dev-xo commented Aug 28, 2024

All right, nothing to worry about @kldeb.

Not sure if we will need some small tests of the base32Encode or if we had those for thirty-two; in any case, let's wait for @mw10013 to share some thoughts, and we will merge it!

@dev-xo
Copy link
Owner

dev-xo commented Aug 29, 2024

We will need to bump @epic-web/totp to the new version, right? Let us know what pending changes need to be made in order for us to also merge this and publish a new version of remix-auth-totp, @kldeb.

Also, thanks for the efforts and the improvements!

@kldeb
Copy link
Contributor Author

kldeb commented Aug 29, 2024

@dev-xo Done. I ran pnpm upgrade on the project so there are a few updates (let me know if this is not ok). All tests pass, ran format, lint, typecheck as well.

@mw10013
Copy link
Collaborator

mw10013 commented Aug 29, 2024

@kldeb Thanks for the PR! Looks good from my side.

@mw10013 mw10013 self-requested a review August 29, 2024 20:03
@mw10013 mw10013 merged commit 486ce6b into dev-xo:main Aug 29, 2024
4 checks passed
@dev-xo
Copy link
Owner

dev-xo commented Aug 29, 2024

Merged and published. Let me know if everything works as expected, @kldeb. Again, thanks for the time spent and the efforts!

@kldeb
Copy link
Contributor Author

kldeb commented Aug 29, 2024

@dev-xo thanks! Just tested it and it works!

@dev-xo
Copy link
Owner

dev-xo commented Aug 29, 2024

Happy to hear that, @kldeb! Thanks for the PR. Feel free to suggest anything else or come up with some other features/improvements!

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.

3 participants