Call execute after load if execute was called before ready

[ZhangYiJiang]

The component has a race condition where if execute() is called before the API is loaded, execute will just fail silently. The fix is to add a flag which we check on load, and calls execute again once the API is loaded if the parent had previously called it. The caller could implement the same fix we are implementing here but it is easier if the component just took care of it automatically. This PR is not compatible with #91

[brdlyptrs]

Why not use onLoad and call execute then?

[ZhangYiJiang]

Do you mean the parent can call execute in onLoad? They absolutely can, but like I said it seems surprising that the component doesn't do this already, and it's very unlikely the caller would not want this to happen.

[brdlyptrs]

@ZhangYiJiang could you provide tests here to illustrate what is expected?

[ZhangYiJiang]

Sure, added a test case that fails on master. It's a bit weird because we need the global hcaptcha not to be defined when the component is mounted to simulate the script loading after the component is mounted, so this is the best I can do

 FAIL  tests/hcaptcha.spec.js
  ● hCaptcha › correctly executes even if execute() was called before load

    expect(received).toBe(expected) // Object.is equality

    Expected: 1
    Received: 0

      163 |         window.hcaptcha = hcaptchaGlobal;
      164 |         instance.handleOnLoad();
    > 165 |         expect(hcaptchaGlobal.execute.mock.calls.length).toBe(1);
          |                                                          ^
      166 |     })
      167 | 
      168 |     describe("Query parameter", () => {

      at Object.<anonymous> (tests/hcaptcha.spec.js:165:58)

Test Suites: 1 failed, 1 passed, 2 total
Tests:       1 failed, 30 passed, 31 total
Snapshots:   0 total
Time:        4.669 s
Ran all test suites.

[ajmnz]

I'm done with #91, and I don't see why it shouldn't be compatible now.

[ZhangYiJiang]

https://github.com/hCaptcha/react-hcaptcha/pull/91/files#diff-bfe9874d239014961b1ae4e89875a6155667db834a410aaaa2ebe3cf89820556R210

This returns Promise<undefined> if !isApiReady - you would expect this to always return a promise resolving to the token, even if the API is not loaded (similar how execute() works after this change).

Fixing this is a bit tricky - you'll probably need something like this

executeAsync() {
  if (!isApiReady) {
    if (this.executePromise == null) {
      this.executePromise = new Promise((resolve) => {
        this.resolveExecutePromise = resolve;
      });
    }
    // This means all calls before isApiReady will resolve to the same token -
    // I assume this is okay, but if it is not we can store an array of promises instead
    return this.executePromise;
  }

  // ...
}

handleOnLoad() {
  // ...
  if (this.resolveExecutePromise != null) {
    resolveExecutePromise(this.executeAsync());
  }
}

Also FWIW given that the only place async is used is here https://github.com/hCaptcha/react-hcaptcha/pull/91/files#diff-bfe9874d239014961b1ae4e89875a6155667db834a410aaaa2ebe3cf89820556R207, which just returns a promise directly, it's might be better to just not add regenerator

[brdlyptrs]

You are going to want to reset the this.executeOnLoad after you call this.execute.

[brdlyptrs]

@ZhangYiJiang You could create a new promise that is returned and then when execute does get called you can resolve when that actual promise is resolved.

[brdlyptrs]

@ZhangYiJiang should we still try to implement this PR, what do you think?

[brdlyptrs]

Hi @ZhangYiJiang looks like I encountered a similar issue as you are trying to solve here. Can you have a look and see if this PR solves the one here?

[ZhangYiJiang]

No that's not quite the same issue - this is calling execute before load, that is calling execute on load. That said, since #91 is merged, this also needs to handle the async case. If execute() is called multiple times, should the same promise be returned, ie. all execute() calls resolve to the same token?