Add ability to abort a kfetch call.

[cjcenizal]

This will be needed for rollups support, since we'll use kfetch to make the search request to the rollups API endpoint, and courier's callClient function requires the ability to abort the promises representing the in-flight search requests.

I tried to add a test asserting that abort actually stopped the promise from resolving, but I don't think the fetch-mock module supports this:

beforeEach(() => {
  fetchMock.mock(matcherName, () => {
    return new Promise((resolve) => {
      setTimeout(() => {
        resolve({ foo: 'bar' });
      }, 300);
    });
  });
});
afterEach(() => fetchMock.restore());

it('should abort the promise when abort() is called', (done) => {
  let isResolved = false;
  const { fetching, abort } = kfetch({ pathname: 'my/path' }, {}, true);
  fetching.then(() => isResolved = true);
  abort();

  setTimeout(() => {
    expect(isResolved).toBe(false);
    done();
  }, 500);
});

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[sorenlouv]

What about attaching abort to the promise like:

fetching.abort = abort;

That way we can also get rid of isAbortable since all requests become abortable and we don't need a secondary api (for abortable requests).

[cjcenizal]

I originally considered this approach but I had a few concerns about monkey-patching a native API:

1. If a native abort method is introduced to promises at some point in the future then we'll need to ensure it either has identical functionality, or make a decision as to whether to cut over to the official method and ensure BWC or to keep the monkey-patched method and have unexpected behavior.

2. Monkey-patching has the potential to confuse consumers as well since this method isn't in the spec, prompting questions like "Is this really a native promise?" and "What other non-spec methods are attached to this promise?" It requires the user to understand the implementation in order to have confidence about what's changed about this promise.

These concerns are mostly based on principle so if you think it's more pragmatic to just monkey-patch the promise, let me know your thoughts because I can definitely be persuaded! OTOH if you think these concerns are worthy then I think creating a secondary API makes sense, because we're highlighting the fact that the consumer is consciously opting into secondary behavior. And realistically, do you think it will be used very often? Do we have many use cases for aborting a fetch?

[sorenlouv]

These are very good points. I was mostly concerned about having two APIs but as you said, there won't be many use cases for aborting fetch, and in those cases it will be a conscious effort.

I like the approach you suggested with a separate kfetchAbortable method.

[sorenlouv]

AbortController is still not supported in Internet Explorer, and our support matrix indicate we support IE11+. Are you planning to add a polyfill?

[sorenlouv]

Maybe abstract this into a separate createAbortable method that returns { signal, abort }

[cjcenizal]

Good idea!

[cjcenizal]

Are you planning to add a polyfill?

Thanks for reminding me! Yes I need to do that.

[cjcenizal]

Alternatively, we could also create a method specifically for creating abortable fetch promises. This way we wouldn't need to alter the kfetch signature or change the API it returns.

export function kfetchAbortable(fetchOptions, kibanaOptions) {
  const abortController = new AbortController();
  const signal = abortController.signal;
  const abort = abortController.abort.bind(abortController);
  const fetching = kfetch({ ...fetchOptions, signal }, kibanaOptions);

  return {
    fetching,
    abort,
  };
}

[cjcenizal]

@nreese @sqren Ready for another look!

CC @jen-huang I've implemented the idea we talked about.

[sorenlouv]

We don't need the Promise constructor anymore, do we? Eg. just revert to what we had (unless you prefer this style ofc :) )

[cjcenizal]

If you don't mind, I would like to keep it in this style because it makes it just a bit more explicit that a promise is being returned (at least to me!)

[sorenlouv]

Very clean. I like it!

[nreese]

lgtm. very nice and easy to follow
code review

[sorenlouv]

Looks good. Feel free to merge regardless of the nits.

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[cjcenizal]

Failure:

21:01:04    │ proc  [ftr]        └- ✖ fail: "dashboard app using current data dashboard snapshots compare area chart snapshot"
21:01:04    │ proc  [ftr]        │        tryForTime timeout: Error: tryForTime timeout: [POST http://localhost:9515/session/f7fe9be3127718180b0940b39ba77ce0/element / {"using":"css selector","value":"[data-test-subj~=\"savedObjectFinderSearchInput\"]"}] no such element: Unable to locate element: {"method":"css selector","selector":"[data-test-subj~="savedObjectFinderSearchInput"]"}

Screenshot:

[cjcenizal]

Retest

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[spalger]

I really like where this ended up!