Let's add fetch to the settled checks?
Techn1x opened this issue · 2 comments
I've been updating an application to be ember 4 ready, part of that involved setting jquery-integration: false
- meaning all existing ember-ajax
and ember-data
calls have switched from ajax to fetch.
Everything works great! Except... for our tests. Basically any test that was previously doing some kind of ajax request and asserting a result now fails, due to not waiting for the request to finish before asserting.
I have some options;
A) use ember-fetch 🟥
- it does some funky stuff, is deprecated and planned to be archived soon, would affect live code instead of just tests, ...nah
B) use ember-test-waiters in a lot of places 🟥
- it would be a lot of work in this app to wrap all store & ember-ajax requests, last resort
C) register fetch promises just like ajax requests with the test waiter system 🟢
- yes please! everything should just work the same when implemented
In theory, an ember app could do something like this before the tests run...
window.fetch_original = window.fetch
window.fetch = (...args) => waitForPromise(window.fetch_original(...args))
but my setup is more complicated, because I use pretender. This is likely a common setup - pretender is used by both mirage and ember-data-factory-guy. Pretender ignores whatever window.fetch is and assigns it with the github fetch polyfill
https://github.com/pretenderjs/pretender/blob/a6d53af7d5d16c3b68670a00aa2b0706a09b6ae6/src/pretender.ts#L123-L129
So we need something a little more robust that sticks around when assigned - as a proof of concept I put the following together in my test suite, and it works well.
import { waitForPromise } from '@ember/test-waiters'
QUnit.testStart(() => {
window.fetch_original = window.fetch // store original fetch so we can restore it later
// keep track of whatever window.fetch has been assigned to, and always return that but wrapped in waitForPromise
// this allows it to be set by pretender at any time, and still be registered with the test waiter system
let fetchNow = window.fetch
function waitForFetch(...args) {
return waitForPromise(fetchNow(...args))
}
Object.defineProperty(window, 'fetch', {
get() {
return waitForFetch
},
set(value) {
fetchNow = value
},
})
})
QUnit.testDone(() => {
// End of test run, do teardown by putting original fetch back.
Object.defineProperty(window, 'fetch', {
value: window.fetch_original,
writable: true,
configurable: true,
})
delete window.fetch_original
})
From here though, I have no idea how to integrate this into ember-test-helpers. If someone would like to give some guidance, or wield the torch, that would be great.
Over in ember-fetch, it seems the agreed upon way forward looks is to handle this in ember-test-helpers's settled()
test helper
ember-cli/ember-fetch#656 (comment)
Monkey-patching fetch
like this is, from my POV, an absolute non-starter, but others on Framework may disagree. (I know it's "just JS" but I loathe things which patch globals.)
My own opinion here is that the right move is:
- APIs which wrap
fetch
should themselves do the test waiter integrations. So Ember Apollo, Ember Data, etc. should handle that. - If you're using "unmanaged"
fetch
, you should wrap it yourself, exactly like you would any other "unmanaged" async operation.
That latter point gets at why I think patching fetch
is the wrong direction: We're not going to monkey patch Promise.then
, but that's the next "obvious" move, and there's no bright line between "replace fetch
" and "replace Promise.then
" in terms of "But it should 'just work'!" They're exactly the same kind of thing, and they introduce a kind of "magic" which is IMO much worse than just importing a tool which does the right thing.
If, in your app, you want a wrapped fetch
that always does this… you can write that.
import { waitForPromise } from '@ember/test-waiters';
export function fetch(...fetchArgs) {
return waitForPromise(window.fetch(...fetchArgs));
}
What's more, that leaves every app in control of their own handling for this, so that if (as you note in your own example here) you need further customization for your own particular test setup, you can just write that. It also makes it really obvious for someone new to that app where and how this is implemented when they want to understand it. One of the great upsides to imports is they give you breadcrumbs to follow.
(Insofar as it's useful to have that exist as a shared behavior, I think there's a reasonable argument that this and only this is what ember-fetch
should supply.)
That all sounds logical to me. I mostly wanted to shine a light here, because it was not fun having to figure out why all my tests were failing, and then it was very difficult finding an explanation until finally finding that one thread in ember-fetch. And then coming up with this code that could deal with what Pretender was doing was another large hurdle. Though that is largely Pretenders' fault for not having a way to provide a custom fetch.
Ultimately there wasn't any kind of "here's what you should do!" kind of answer, anywhere.
I hope this github issue here helps others out, because I don't think my setup is anything unusual.