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

[ Feat ]: Pass a pre-read FormData object via context. #60

Merged
merged 4 commits into from
May 10, 2024

Conversation

themkvz
Copy link
Contributor

@themkvz themkvz commented May 2, 2024

When I try to read FormData (for validation or something else) in action I get an error
FetchError: Invalid response body while trying to fetch http://localhost:5173/login: Invalid state: ReadableStream is locked

login.tsx

export async function action ({ request }: ActionFunctionArgs) {
  const formData = await request.formData()
  const type = formData.get('type') as string

  if (!type) {
    return json({ message: 'Type is required' }, { status: 400 })
  }

  const successRedirect = type === 'form' ? '/account' : '/verify'

  await authenticator.authenticate(type, request, {
    successRedirect,
    failureRedirect: '/login',
    context: { formData }
  })
}

As a solution, I added simple checking if we have already passed formData via options

private async _readFormData(request: Request, options: AuthenticateOptions) {
    if (request.method !== 'POST') {
      return new FormData()
    }

    if (options.context?.formData instanceof FormData) {
      return options.context.formData
    }

    return await request.formData()
  }

@dev-xo
Copy link
Owner

dev-xo commented May 2, 2024

Hello @themkvz.

We used to have an example on how to handle errors with remix-auth, sadly removed it some time ago. Probably looking into remix-auth examples and how to handle errors could help with your current situation?

Can you please provide more details of your case and how the Pull Request will affect the library?

Two more things:

  • pnpm-lock.yaml has been modified somehow; not sure if you can explain why. Otherwise, please undo the changes.
  • Also, I would suggest fixing the indentation, as it doesn't seem to be the one originally provided by the repository. Our current .prettier config could help with it.

Thoughts on this issue @mw10013?

@dev-xo dev-xo requested a review from mw10013 May 2, 2024 16:32
@mw10013
Copy link
Collaborator

mw10013 commented May 2, 2024

@themkvz: Thank you for the PR. remix-auth-totp is designed to take in a pristine request. We're cautious about providing another mechanism to convey the formData so as not to add more complexity to the API.

I wonder if Request.clone() may be helpful for your use case (https://developer.mozilla.org/en-US/docs/Web/API/Request/clone). Please share your thoughts.

It is curious to see logic in the action of your login route that may redirect to an /account route. Upon success the login action would generally redirect to a verify route so that the user can input the totp code as in the example project snippet below.

https://github.com/dev-xo/totp-starter-example/blob/4f14dbc1696a81827f2708a88e1e16beec176edc/app/routes/login.tsx#L26-L39

@themkvz
Copy link
Contributor Author

themkvz commented May 3, 2024

Thank you for your response!

@dev-xo I will revert pnpm-lock.yaml. Which indents do you mean? My VSCode uses your . prettierrc config, with "tabWidth": 2 and "singleQuote": true

@mw10013 I use multiple forms on a single remix page following the next way https://sergiodxa.com/articles/multiple-forms-per-route-in-remix (using hidden input with name="type"), I need two forms because, in my Login page, I have two auth methods/strategy (authenticator.use(totpStrategy, "TOTP").use(formStrategy, "form");) – first is a traditional method with user email and user password, and second one is your based on the magic link.
Depending on the filled form we need to pass the correct type to authenticator.authenticate
Provided solution with a pre-read FormData object I use from this package https://github.com/sergiodxa/remix-auth-form?tab=readme-ov-file#passing-a-pre-read-formdata-object

@mw10013
Copy link
Collaborator

mw10013 commented May 4, 2024

@themkvz: Thanks for providing more color on your use case. It is helpful and compelling.

Can you add a test for this? Also, a small section at the end of docs/customization.md explaining that this exists and why you might need it. Thanks!

@dev-xo dev-xo changed the title [ Fix ]: Passed a pre-read FormData object via context [ Feat ]: Pass a pre-read FormData object via context. May 7, 2024
@dev-xo
Copy link
Owner

dev-xo commented May 7, 2024

Any news on this @themkvz? Otherwise I think we can close it for now, and get back to it later at some point.

@themkvz
Copy link
Contributor Author

themkvz commented May 7, 2024

Any news on this @themkvz? Otherwise I think we can close it for now, and get back to it later at some point.

I have already added two simple tests and I plan to add a short section with an explanation in the docs

@dev-xo
Copy link
Owner

dev-xo commented May 7, 2024

Thanks @themkvz, happy to know this is still active. Take your time and let us know when we can review it.

  • Feel free to keep the docs explanation simple, as I will probably review it and adapt it for the docs.

@themkvz
Copy link
Contributor Author

themkvz commented May 8, 2024

@dev-xo I've added the brief description to the README.md
Before pushing I ran format script (pnpm run format) and I see some additional changes in README.md, if it is a problem – let me know

About test:
I've already added two short tests

@dev-xo
Copy link
Owner

dev-xo commented May 8, 2024

To be clear @themkvz, your current issue (and PR) could be solved by cloning the request like request.clone(), or not?

  • If that's the case, this is more of a syntactic sugar approach, and something that we could simply document about.

Also, these changes will now imply that passing a pre-read FormData to the authenticate method will be required, right?

@themkvz
Copy link
Contributor Author

themkvz commented May 8, 2024

To be clear @themkvz, your current issue (and PR) could be solved by cloning the request like request.clone(), or not?

It's an additional option

Because in the Remix docs, for example, here we have an example of how to use request.formData(), on the other hand, the search through the Remix docs for request.clone() does not give anything, in the whole the Remix docs any mentions about request.clone() (I understand that is the native Web API).

The user has two options (if he has this specific case with more than one strategy on the same page and needs validate some form fields before passing the request):

  1. Use const formData = await request.clone().formData()
  2. Use the usual const formData = await request.formData() and pass a pre-read FormData to the context

Also, these changes will now imply that passing a pre-read FormData to the authenticate method will be required, right?

If you need to validate FormData, yes – it will be required


I opened this PR because I saw the section on how of works with a pre-read FormData in another remix-auth strategy, that's all)

@dev-xo
Copy link
Owner

dev-xo commented May 8, 2024

All good @themkvz.

I was wondering if we could simply document this in the docs, letting users know that if they are looking to handle multiple auth strategies or validate the form, they could simply clone the request.

Ideally, the simpler we keep the strategy, the fewer bugs we could introduce, and the easier it will be to maintain it.

  • I'll leave this decision to @mw10013, as he probably has more insights on how useful this could be overall.

I appreciate the efforts in improving the strategy and making it simpler for everyone else to handle this situation or similar ones, so thank you!

@themkvz
Copy link
Contributor Author

themkvz commented May 8, 2024

All good @themkvz.

I was wondering if we could simply document this in the docs, letting users know that if they are looking to handle multiple auth strategies or validate the form, they could simply clone the request.

Ideally, the simpler we keep the strategy, the fewer bugs we could introduce, and the easier it will be to maintain it.

  • I'll leave this decision to @mw10013, as he probably has more insights on how useful this could be overall.

I appreciate the efforts in improving the strategy and making it simpler for everyone else to handle this situation or similar ones, so thank you!

Yeah, sure, it will be maybe the best way to post this possibility in the docs
I had some struggle when I got the next error FetchError: Invalid response body while trying to fetch http://localhost:5173/login: Invalid state: ReadableStream is locked :)

@mw10013
Copy link
Collaborator

mw10013 commented May 10, 2024

@themkvz: Thanks for the additions. We'd like to proceed with this. I ran gh pr checkout 60 locally and made a few local edits that I want to push to your PR with git push https://github.com/themkvz/remix-auth-totp.git HEAD. I'm getting

 ! [remote rejected] HEAD -> themkvz/main (permission denied)
error: failed to push some refs to 'https://github.com/themkvz/remix-auth-totp.git'

Can you grant me permission? Thanks.

@mw10013 mw10013 merged commit d596754 into dev-xo:main May 10, 2024
4 checks passed
@dev-xo
Copy link
Owner

dev-xo commented May 10, 2024

Reverted it back, as it was not squashed and the whole history commit was cluttered with small unnecessary commits.

  • Will try to merge it back as a squashed commit.

Otherwise, I will open a PR myself with the updated changes and push it.

@dev-xo
Copy link
Owner

dev-xo commented May 10, 2024

#64 Squashed, merged & published v3.3.0.

Let me know if this settles the issue for you @themkvz. Also, thanks for the contribution! 👍🏻

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