[component][typeahead-suggestion] Add network client for typeahead-search component

[berndsi]

• Add fetch wrapper (only supports native fetch at this time)
• Add REST API search client

This network client provides fetchByTitle(), to be used for typeahead search suggestions.
Not providing accept-language header at this time since it does not seem to make a difference.

By default the requests are mocked, so no network is used.
If the environment variable TEST_LIVE_REQUESTS is set to a truthy value, the mocks are
disabled and requests are made against a live server.

Bug: T244287

[niedzielski]

Looks good! 👍 A couple questions inline.

[niedzielski]

As you noted, we know we'll need to support IE11. How best to go about it? These are some of the concerns that come to mind:

• We want to be able to easily test either in implementation.
• This client is currently touching the Window global. State and especially globals can be problematic but is this ok?
• IE11 also doesn't support Promises.
• The library will be loaded and configured once but may be depended upon by multiple codebases, not just Vector.

Did you consider passing window.fetch / $.ajax / custom Fetch implementation as a dependency of the SearchClient, along with query and host, instead of creating this utility function? Passing fetch would allow you to avoid relying on the window global and allow to test any implementation you wanted.

[berndsi]

Support for IE11 is tough here without adding to the bundle size. Not sure how much bloat to the bundle size is acceptable here.
Passing in a fetch implementation seems problematic since window.fetch and $.ajax behave slightly differently. We would have to wrap the $.ajax call if we wanted to make it behave like fetch.
IE11 not supporting Promises throws in another wrench. Makes me wonder if I should drop fetch for now and go with callbacks and XHR. (So far I only see await used in a few tests, beside what this PR adds.)
I wonder if it's worth doing a prototype using a ponyfill for Promises and fetch at all, to see how much it adds to the bundle size.

[niedzielski]

I think the jQuery promises are actually ES6 compatible? I think it'd be ok if Vector unconditionally passed $.ajax in if that simplifies things. We just shouldn't assume the fetch implementation in the library.

https://jquery.com/upgrade-guide/3.0/#breaking-change-and-feature-jquery-deferred-is-now-promises-a-compatible
https://www.mediawiki.org/wiki/JQuery

[niedzielski]

If I understand correctly, fetch is not a requirement we can demand due to the support requirements. Our options are:

1. Ask the client to pass in a fetch implementation (likely to be jQuery). Unofficial note on some differences.
2. Pony/polyfill fetch and Promise. This will increase our bandwidth usage and may still behave differently from native fetch. For instance, see these caveats for the Preact guy / MDN's implementation.
3. Use XHR. Callbacks but on the plus side we cancel requests.
4. Something else?

@speedandfunction-anribolon, thoughts on using an XMLHttpRequest?

[speedandfunction-anribolon]

@niedzielski @berndsi I wouldn't use XMLHttpRequest in spite of it's a bulletproof solution. I'd better stick with polyfills. Maybe we can deliver separate bundles for IE including polyfills for fetch and promises and "polifill-less" bundle for the rest of browsers?

[niedzielski]

I haven't used this but es6-promise is about 2.4 kB and unfetch is about .5 kB. Thoughts? /cc @speedandfunction-anribolon

[berndsi]

I was also considering the 'core-js/modules/es6.promise' part of core-js, as described in Loading Polyfills Only When Needed. The alternative for unfetch was whatwg-fetch but that is quite a bit larger at 3.1kB.
Before we drill down more into various options I thought there was a more general concern about having an extra bundle for older browser, was there not?

[niedzielski]

Let's move this discussion to Slack so Web can weigh in.

[berndsi]

@niedzielski Should we merge the fetch implementation, then create a separate PR for XMLHttpRequest? That way we could revert the latter commit to get back to a pure fetch implementation. If this is the case, are there any other open issues with this PR?

Side note: there is now a new Phab task to add a fetch polyfill to mediawiki core.

[niedzielski]

Merging with the expectation that we will use XHR or polyfill ASAP.

[niedzielski]

Do you think we need these tests? I'm unsure. When it's not mocked, we test the actual Fetch implementation but that's going to be either window.fetch() or $.ajax(), neither of which is ours.

[berndsi]

Originally I had this hitting example.org and had them as more general fetch calls. Now that I switched to something very similar to what the search client does the 200 response tests do seem redundant. I'd probably still prefer to keep the 404 test. I was actually considering adding some more tests for network issues since the jQuery version does behave quite differently. (It throws exceptions, uses different fields than fetch.)

[niedzielski]

Why does this appear in both beforeEach and afterAll?

[berndsi]

The motivation for the one in afterAll() was to undo what was done in beforeAll().

Not exactly sure what's going on here. The comment in the d.ts file says

// alias of fetchMock.disableMocks() for ease of ES6 import syntax to not clash with other libraries

When I try using just fetchMock.disableMocks() instead of jestFetchMock.disableFetchMocks() that works but we lose the symmetry of beforeAll and afterAll. When also replacing the enableMocks call it forces me to change the jestFetch mock import. I tried just import 'jest-fetch-mock' but that fails the tests: ReferenceError: fetchMock is not defined.

[niedzielski]

I haven't used jest-fetch-mock (or at least don't remember having used it). I'm looking at the docks and wondering if you considered using mockOnce() or mockOnceIf(). It seems like those might be cleaner.

[berndsi]

The docs of mockOnce() seems to imply that it always mocks:

but guarantees the next call to fetch will be mocked even if the default behavior of fetchMock is to use the real implementation

I think it would be nice to have the option to run these tests against a real server to keep these grounded in reality, and not just make up some responses that are unlikely to happen in the real world.

[niedzielski]

I see but wouldn't this move to just before the call to fetchByTitle() in the tests themselves? For example, fetch.doMockOnceIf( mockedRequests ).

This isn't a big deal on it's own but I wanted to encourage you to take the time to write the best network tests here as possible since others contributors are likely to copy your approach. At the other extreme, I remember in the Android app we had trouble with tests depending on other tests running in a certain order or being unable to run in parallel because of configuration changes to globals. I think the before/after parity you've given here will help us avoid those issues.

Then again, maybe all this breaks if change the underlying implementation from fetch to XHR 😭

[berndsi]

@niedzielski I'm not sure I understand the first paragraph of your most recent comment. fetchMock.mockResponse() is right before the call to fetchByTitle(). Are you asking to just replace it with fetch.doMockOnceIf() or something else?

Yes, if we change to implementation away from fetch this all seems mood.

[berndsi]

I've changed this to use mockOnce now.

[niedzielski]

If I understand correctly, fetch is not a requirement we can demand due to the support requirements. Our options are:

1. Ask the client to pass in a fetch implementation (likely to be jQuery). Unofficial note on some differences.
2. Pony/polyfill fetch and Promise. This will increase our bandwidth usage and may still behave differently from native fetch. For instance, see these caveats for the Preact guy / MDN's implementation.
3. Use XHR. Callbacks but on the plus side we cancel requests.
4. Something else?

@speedandfunction-anribolon, thoughts on using an XMLHttpRequest?

[speedandfunction-anribolon]

Looks good to me!

[berndsi]

@niedzielski Do you know why changing this to '@/http/fetch' doesn't work? I'm getting

    Cannot find module '@/http/fetch' from 'src/components/typeahead-search/http/restApiSearchClient.ts'

    Require stack:
      src/components/typeahead-search/http/restApiSearchClient.ts
      src/components/typeahead-search/http/restApiSearchClient.test.ts

    > 1 | import { buildQueryString, fetch } from '@/http/fetch';
        | ^
      2 | import { SearchClient, SearchResponse } from './SearchClient';
      3 | 
      4 | // https://www.mediawiki.org/wiki/API:REST_API/Reference#Search_result_object

      at Resolver.resolveModule (node_modules/jest-resolve/build/index.js:308:11)
      at Object.<anonymous> (src/components/typeahead-search/http/restApiSearchClient.ts:1:1)