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

Let's add fetch to the settled checks? #1275

Open
Techn1x opened this issue Dec 1, 2022 · 2 comments
Open

Let's add fetch to the settled checks? #1275

Techn1x opened this issue Dec 1, 2022 · 2 comments

Comments

@Techn1x
Copy link

Techn1x commented Dec 1, 2022

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 🟥

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)

@chriskrycho
Copy link
Contributor

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:

  1. APIs which wrap fetch should themselves do the test waiter integrations. So Ember Apollo, Ember Data, etc. should handle that.
  2. 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.)

@Techn1x
Copy link
Author

Techn1x commented Dec 1, 2022

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.

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

No branches or pull requests

2 participants