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

return token promise by recaptcha.execute #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

f1qwase
Copy link

@f1qwase f1qwase commented Jan 19, 2018

after this changes you can use recaptcha.execute this way:

this.recaptcha.execute()
  .then(recaptchaToken => submitForm(recaptchaToken))

so you dont't need onResolved and getResponse methods

@szchenghuang
Copy link
Owner

I like the idea. However, the callback will be executed twice when reCAPTCHA is resolved successfully, wouldn't it?

@f1qwase
Copy link
Author

f1qwase commented Jan 22, 2018

to avoid this I just pass the 'noop function' as an onResolved prop to Recaptcha component

<Recaptcha
  ref={_ => this.recaptcha = _}
  sitekey='***'
  onResolved={() => null}
/>

it is not very nice but doesn't break old API
may be onResolved should be declared as non-required, if you like this idea, tell me, I'll update this request

@szchenghuang
Copy link
Owner

Please do. I do like the idea. I will follow up after your update. Thanks.

@Palisand
Copy link

Palisand commented Mar 19, 2018

TL;DR

This promise is not a good idea due to Google reCAPTCHA's lack of an error callback, which would provide the only proper mechanism for promise rejection. Without it, we risk forever-pending promises. An alternate, non-prop means of assigning window[this.callbackName] should be used instead.

If execute fails (if there is no internet connection for instance) then this promise will fail to resolve and will remain in a pending state indefinitely. execute will not throw an error, it will only result in an alert on failure, so there doesn't appear to be a proper way in which you would return a rejected promise. Rejecting the promise with a timeout will not work for scenarios where a challenge is presented; there's no telling how long a user will take to solve the challenge.

As an alternative, you can dip your toes in callback hell and add an onResolved method to GoogleRecaptcha that would assign window[this.callbackName] and, in turn, overwrite props.onResolved:

class GoogleRecaptcha extends Component {
  ...
  componentDidMount() {
    ...
    this.onResolved = action => window[this.callbackName] = action;
    ...
  }
  ...
}

And in your validation & submission flow, something like this:

const submitArgs = [foo, bar, baz];
if (googleRecaptchaRef) {
  googleRecaptchaRef.onResolved(() => {
    submit(...submitArgs, googleRecaptcha);
  });
  googleRecaptcha.execute();
}
else {
  submit(...submitArgs);
}

function submit(foo, bar, baz, googleRecaptcha) {
  ...
  const googleRecaptchaResponse = googleRecaptcha && googleRecaptcha.getResponse();
  ...
}

@szchenghuang
Copy link
Owner

@Palisand, you make a valid point. Your comment is visionary. I do hesitate regarding the possible pending promise. I tend not to make a merge with a heuristic.

Sorry for the late response.

@szchenghuang szchenghuang force-pushed the master branch 9 times, most recently from 885a65a to 9c7f8cb Compare May 10, 2021 12:15
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.

None yet

3 participants