dev-xo / remix-auth-totp

A Time-Based One-Time Password (TOTP) Authentication Strategy for Remix-Auth.
https://totp.fly.dev
MIT License
398 stars 26 forks source link

[ Fix ]: Docs suggest "failureRedirect" as optional but throws Error when removed. #66

Closed justin-hackin closed 3 months ago

justin-hackin commented 3 months ago

In the example here it is stated:

// The failureRedirect route will be used to render any possible error. // If not provided, ErrorBoundary will be rendered instead.

When it is removed from the options, we get Error: Missing required failureRedirect URL. in a case that would normally succeed. I assumed what this comment meant is that failures will just throw instead of redirecting. Was that the intent?

dev-xo commented 3 months ago

Good catch @justin-hackin, if you want to contribute to fixing that comment, it will be more than welcome. Otherwise, I will look into it whenever I get a moment.

Thank you for the catch!

justin-hackin commented 3 months ago

Good catch @justin-hackin, if you want to contribute to fixing that comment, it will be more than welcome. Otherwise, I will look into it whenever I get a moment.

Thank you for the catch!

Hi, thanks for the invite. I'm pretty confused about how this library works (e.g. why is authenticate called on both login and verify in the example and what's going on under the hood). I don't think I'll be able to contribute.

chiptus commented 3 months ago

@dev-xo do you expect the docs to be fixed? or the actual code?

dev-xo commented 3 months ago

I think the issue is about the docs not matching the actual code behavior, at least it's what I'm able to understand. The fix should be simple (docs related), sadly I'm kinda busy, although I can look into it anytime soon.

Not sure if that's your question @chiptus, would appreciate some specification on it.

chiptus commented 3 months ago

as you said docs are not matching the code. should the docs be fixed? or the code to match the docs?

dev-xo commented 3 months ago

Code should be working as expected, so in this case, probably the docs should be fixed. Not sure if you want to contribute with a little PR, but if that's the case, it will be welcomed! ;)

failureRedirect should be required, and seems to be specified as optional in those comments. At least in the example provided by @justin-hackin, related to one of our starter templates.

dev-xo commented 3 months ago

Thanks for noticing this, @justin-hackin. If there's anything else, let me know 👍🏻