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

Implement async hcaptcha execute #91

Merged
merged 19 commits into from
Oct 20, 2021
Merged

Conversation

ajmnz
Copy link
Contributor

@ajmnz ajmnz commented Jul 14, 2021

Implements #55

Extras

  • Adds watch script in package.json
  • Adds Babel preset plugin-transform-runtime to deal with await, async and promises

Todo

  • Clean example React app and add async example properly
  • Handle error and close.
  • Add tests

Help/Feedback appreciated

src/index.js Outdated Show resolved Hide resolved
@ajmnz ajmnz marked this pull request as ready for review July 14, 2021 19:10
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
@ajmnz ajmnz marked this pull request as draft July 14, 2021 19:12
@ajmnz ajmnz marked this pull request as ready for review July 16, 2021 22:42
@ajmnz
Copy link
Contributor Author

ajmnz commented Jul 16, 2021

@brdlyptrs Done. Not sure how to approach tests tbh.

@ajmnz ajmnz changed the title [WIP] Implement async hcaptcha execute Implement async hcaptcha execute (#55) Jul 16, 2021
@ajmnz ajmnz changed the title Implement async hcaptcha execute (#55) Implement async hcaptcha execute Jul 16, 2021
@ajmnz ajmnz requested a review from brdlyptrs July 17, 2021 09:20
@brdlyptrs
Copy link
Collaborator

@ajmnz Since the hcaptcha.execute method does not return anything at the moment you could actually move all this code from the new executeAsync method to the react class exposed execute method.

Something to consider is creating and exposing a custom Error. Something like hCaptchaError, where you can create a new instance and check for it either in tests or the application that is running hCaptcha.

Testing

You are going to want to use the instance created in the test file and mostly do the following:

it("Async execute should return token with test sitekey", async () => {
        const { token } = await instance.execute();
        expect(token).toBe("10000000-aaaa-bbbb-cccc-000000000001");
});

Since you are also storing the reject you could test the failure cases with something like:

it("Async execute should reject with correct message", async () => {
       const instance = ReactTestUtils.renderIntoDocument(
            <HCaptcha
                sitekey={TEST_PROPS.sitekey}
                onOpen={() => instance.reject(new Error("hCaptcha expired"))}
            />);
        try {
            await instance.execute();
        } catch (e) {
            expect(e.message).toBe("hCaptcha expired");
       }
});

@ajmnz
Copy link
Contributor Author

ajmnz commented Jul 26, 2021

@brdlyptrs Everything is done except for tests. For some reason callbacks aren't firing and the promise never gets resolved/rejected, triggering a Jest timeout error. Everything works fine in the examples though.

If you have time, please pull the code and see if you can give me a hand here.

@brdlyptrs
Copy link
Collaborator

@ajmnz Sorry for the late reply, I wanted to let you know we will be deploying an option to make hcaptcha.execute async natively this week. We most likely can adjust this PR to match this update internally.

@ajmnz
Copy link
Contributor Author

ajmnz commented Aug 18, 2021

@ajmnz Sorry for the late reply, I wanted to let you know we will be deploying an option to make hcaptcha.execute async natively this week. We most likely can adjust this PR to match this update internally.

Awesome, will wait for any updates

@brdlyptrs
Copy link
Collaborator

@ajmnz We've updated the native JS and it is now live. We should be updating the docs soon to reflect this change. Here is an example:

const { response, key } = await hcaptcha.execute(id, { async: true });

@ajmnz
Copy link
Contributor Author

ajmnz commented Sep 22, 2021

Hey @brdlyptrs 👋

Sorry for the delay, I've had a pretty crazy month. I've implemented a new method that passes { async: true } to the native execute method, together with its tests, types and examples.

src/index.js Outdated Show resolved Hide resolved
@ajmnz
Copy link
Contributor Author

ajmnz commented Sep 27, 2021

@brdlyptrs I think we're ready to merge now.

Here's a TS Playground with the overloads.

Let me know if you need any more changes.

src/index.js Outdated Show resolved Hide resolved
@brdlyptrs
Copy link
Collaborator

@ajmnz this looks good, nice job and good tests! Last thing we need to do is bump the version number.

@ajmnz
Copy link
Contributor Author

ajmnz commented Sep 29, 2021

@brdlyptrs Done

@pagarazzi
Copy link

ping. is there any more work on this? It is worth noting that this is the behavior reflected in the official hCaptcha docs, and this library's example points to official hCaptcha docs, so it is misleading if this behavior is not yet integrated.

@ajmnz
Copy link
Contributor Author

ajmnz commented Oct 16, 2021

@pagarazzi
Everything is done on my end, but I don't know why they didn't merge. The execute method still works the same if you don't pass async: true to it though.

@ajmnz
Copy link
Contributor Author

ajmnz commented Oct 16, 2021

@brdlyptrs
Also consider #99

@pagarazzi
Copy link

pagarazzi commented Oct 16, 2021

@ajmnz I built your library but couldn't get it to work. There must have been some transpiling error or an error in the code.

Only the first parameter captchaId is effectively being passed to the lower level .execute() function.
Though the function intends to pass both captchaId, opts, the hCaptcha type only allows for one https://github.com/ajmnz/react-hcaptcha/blob/async-execute/types/index.d.ts#L39
After fixing it manually in the dist/ files I could get it to work. However, only fixing the type didn't fix the build after another transpilation. Am I missing something?

The official library asks for passing captchaId, opts. Already the higher level execute() from the react wrapper only allows for opts. Is there a binding functions in between that I am missing? Shouldn't it be execute(widgetId, opts = null) in https://github.com/ajmnz/react-hcaptcha/blob/async-execute/src/index.js#L199 ?

@ajmnz
Copy link
Contributor Author

ajmnz commented Oct 16, 2021

@pagarazzi

the hCaptcha type only allows for one

The official library asks for passing captchaId, opts. Already the higher level execute() from the react wrapper only allows for opts. Is there a binding functions in between that I am missing?

The low level execute comes from the "official" hCaptcha API, which has nothing to do with this library. What react-hcaptcha offers is an execute method that takes an object as its only optional parameter (opts), and takes care of calling the execute function from the hCaptcha API. The execute method in this library will take care of passing the captchaId to the original execute method. So yeah, you only have to pass opts.

@pagarazzi
Copy link

@ajmnz I understand that but it is not intuitive as it goes against the docs in the official library, which are the only docs existent.

Also the usage was pointed out here #91 (comment) and I assumed that was meant for this library.

If we want to keep it to one parameter we should mention it at least in the README, otherwise many people will stumble upon this
https://github.com/ajmnz/react-hcaptcha/blob/async-execute/README.md#methods

@IvanSaiz
Copy link

@ajmnz @brdlyptrs Any news with this? This feature would help me a lot!

@brdlyptrs
Copy link
Collaborator

This looks good @ajmnz. Let's wait to bump to version 1.0 after this PR once we know all is good.

@brdlyptrs brdlyptrs merged commit 02c6518 into hCaptcha:master Oct 20, 2021
@brdlyptrs
Copy link
Collaborator

Thanks for all the help @ajmnz again.

@ajmnz
Copy link
Contributor Author

ajmnz commented Oct 20, 2021

Thanks for all the help @ajmnz again.

Anytime! This was fun 🤘

@ajmnz
Copy link
Contributor Author

ajmnz commented Oct 27, 2021

@brdlyptrs Could you trigger a release for 0.3.8? I've been waiting for this feature for some time. Thanks!

@nicoleIOB
Copy link

nicoleIOB commented Nov 3, 2021

Could you guys help me? I can't seem to make the async execute to work. I even tried the example provided by you here but the "Verified asynchronously: ", res) it is coming undefined.
I don't know what I'm missing. Thanks!

@ajmnz
Copy link
Contributor Author

ajmnz commented Nov 3, 2021

Could you guys help me? I can't seem to make the async execute to work. I even tried the example provided by you here but the "Verified asynchronously: ", res) it is coming undefined. I don't know what I'm missing. Thanks!

I think I missed updating the example.

This should work

const res = await captchaRef.current.execute({ async: true });
console.log("Verified asynchronously: ", res);

@nicoleIOB
Copy link

Tried that as well but it isn't working 😞 . It's like it doesn't wait for the execute fn to execute the captcha

@ajmnz
Copy link
Contributor Author

ajmnz commented Nov 3, 2021

Tried that as well but it isn't working 😞 . It's like it doesn't wait for the execute fn to execute the captcha

Are you on version 0.3.8?

@nicoleIOB
Copy link

Now it looks like it's working. I think the compiler was getting the previous version. Thank you so much!!

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.

5 participants