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(execute): Handle async opts #238

Merged
merged 8 commits into from
Aug 14, 2024
Merged

Conversation

gabrielferreira-imi
Copy link
Contributor

@gabrielferreira-imi gabrielferreira-imi commented Aug 8, 2024

Fixes #237

@gabrielferreira-imi gabrielferreira-imi requested a review from a team as a code owner August 8, 2024 23:23
@gabrielferreira-imi gabrielferreira-imi self-assigned this Aug 8, 2024
@brdlyptrs brdlyptrs changed the title Fix(execute): Handle async opts fix(execute): Handle async opts Aug 8, 2024
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Show resolved Hide resolved
@brdlyptrs
Copy link
Collaborator

@gabrielferreira-imi code looks good can you add a test to confirm that the execute command is stored and called after hCaptcha onload is executed.

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

Thanks @gabrielferreira-imi!

Need to add one more test to verify the onload callback will also process the ready function correctly. Once that is done, can you bump the version in the package.json?

brdlyptrs
brdlyptrs previously approved these changes Aug 14, 2024
Copy link
Collaborator

@brdlyptrs brdlyptrs left a comment

Choose a reason for hiding this comment

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

@zoryana94 mind confirming on your end that all is good as well?

Copy link
Contributor

@zoryana94 zoryana94 left a comment

Choose a reason for hiding this comment

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

Facing the issue while executing hCaptcha on the component mount
Suggested changes seem to fix it!

this._onReady = (id) => {
try {
if (opts && opts.async) {
hcaptcha.execute(id, opts).then(resolve).catch(reject);
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like const hcaptcha = this._hcaptcha; needs to be also added inside of the this._onReady function
after making this change, it seems to start working:

this._onReady = (id) => {
      try {
        const hcaptcha = this._hcaptcha;

        if (opts && opts.async) {

otherwise, I receive this when executing hCaptcha on the component mount:

Screenshot 2024-08-14 at 10 43 37

src/index.js Outdated
@@ -327,22 +329,38 @@ class HCaptcha extends React.Component {
}

execute (opts = null) {

opts = typeof opts === 'object' && opts !== null ? opts : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

opts !== null check seems to be redundant
as opts are set to null anyway

opts = typeof opts === 'object' ? opts : null;

Copy link
Contributor

@zoryana94 zoryana94 left a comment

Choose a reason for hiding this comment

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

Tested on multiple test apps
Should be good now, @gabrielferreira-imi 👍

@gabrielferreira-imi gabrielferreira-imi merged commit 1066685 into master Aug 14, 2024
4 checks passed
@gabrielferreira-imi gabrielferreira-imi deleted the fix/hcaptcha-execute branch August 14, 2024 16:58
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.

.execute sometimes returns undefined instead of a Promise
4 participants