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

Archive this repo? #656

Open
sandstrom opened this issue Sep 8, 2021 · 16 comments
Open

Archive this repo? #656

sandstrom opened this issue Sep 8, 2021 · 16 comments

Comments

@sandstrom
Copy link

sandstrom commented Sep 8, 2021

Now that Ember 4.0.0 is out and IE 11 is no longer supported, maybe it's easiest to just archive this repo, with a note in the readme explaining that it has served it's purpose.

Possibly coupled with a deprecation notice when this addon is used in an Ember >= 4.0.0 project

People can still use it, but we shouldn't spend more time on it.

https://caniuse.com/fetch

@robclancy
Copy link

Would this not still be useful because of the imports?

@sandstrom
Copy link
Author

@robclancy You don't have to import fetch in a modern browser. It's part of the standard library (Web APIs).

@mansona
Copy link
Member

mansona commented Sep 14, 2021

I think this project would still have a large influence on fastboot apps 🤔 since fetch is a web api and it hasn't been added to V8 or Node. I don't know the specifics of this but I get the feeling that archiving this project would be a bit extreme

@sandstrom sandstrom changed the title Archive this repo Archive this repo? Sep 14, 2021
@kboucher
Copy link

kboucher commented Sep 16, 2021

IE 11 is still supported by Microsoft and it will be at least another nine months before we can reasonably start dropping support for it. This request is premature.

@MrChocolatine
Copy link

IE 11 is still supported by Microsoft and it will be at least another nine months before we can reasonably start dropping support for it. This request is premature.

To complement this answer:

https://docs.microsoft.com/en-us/lifecycle/faq/internet-explorer-microsoft-edge

The Internet Explorer 11 desktop application will go out of support for certain operating systems starting June 15, 2022

@ef4
Copy link
Contributor

ef4 commented Feb 16, 2022

I think this project would still have a large influence on fastboot apps

I don't think this is a good argument for keeping ember-fetch, because:

  1. We can provide fetch to fastboot via a global just as easily as we can via an import. That could just become part of fastboot itself, as setting up a browser-like environment to run the ember app in is fastboot's job.
  2. Even node is planning to add a built-in fetch global. feat: add build for porting fetch to Node.js nodejs/undici#1183

@raido
Copy link

raido commented Apr 29, 2022

I am not sure if this package can simply be deprecated without migration guide. I tried to remove it from my app with ember-concurrency tasks and the moment I switch to native fetch, we no longer have runloop wrapping and tests simply fail, because component instances etc. in tests don't wait for async things to finish.

Sure I can use https://github.com/emberjs/ember-test-waiters#waitforpromise-function for example to wrap things for test waiter to understand that there is async work still pending but it's not as simple as yarn remove ember-fetch.

--

It might be easier to create wrapped fetch() just for tests, like in setupApplicationTest() test or similar call site.

--

This package could exist for transition period as wrapper provider for native fetch just for test waiter purposes 🤔 .

@NullVoxPopuli
Copy link
Contributor

Node 16 or.. 17? Now has fetch. Is it time to archive?

@ef4
Copy link
Contributor

ef4 commented Jun 2, 2022

Archiving this is not the actual work required. Somebody needs to ship the replacement that keeps everybody's test suites still working with the same behavior as before.

Probably that code should still just live in this package. The only change apps should be told to do is to delete the actual import statement. You would still want the ember-fetch dependency because it would:

  • during tests, wrap native fetch in @ember/test-waiters. Without this, arbitrarily large refactorings would be needed to keep people's test suites passing
  • during fastboot, polyfill fetch in our currently supported Node versions that don't already have it

Done that way, it wouldn't even require a breaking change. You could do a minor. It would deprecate importing fetch from ember-fetch, with a message telling people to just delete the import statement. That makes it an easy upgrade.

@simonihmig
Copy link

I believe that currently it would only properly integrate with the test waiters when using the import, not? That's at least how I read this code

Given that the addon

  • uses so much broccoli magic (just look at index.js)
  • and other APIs that IMHO should eventually go away / be deprecated (like explicit define() usage, or Ember.Test)
  • in that shape is hardly migratable to v2 format (I assume?)

can't we instead let users that don't need it anymore - as they use modern browsers + node 16 (in case of FastBoot) - just use the global DOM API, and add some magic to @ember/test-helpers's settled() so that it is able to add test-waiter tracking of native fetch. It does already a bunch of settledness checks (including support for jQuery.ajax 😁).

So for "modern" apps, they would just use "the platform", not have to install additional addons, but still have proper test integration.

@ef4
Copy link
Contributor

ef4 commented Jul 3, 2022 via email

@robinborst95
Copy link

I have a question about the tests wrapping fetch in a test waiter, which is also why we use this addon. It might be worth filing a separate issue, but I think it's also relevant to mention in the context of this issue, because it has to do with the tests (not) waiting.

For testing, we use PollyJS, which also does some wrapping around the fetch function (in the fetch adapter). This resulted in the tests not properly waiting for the outcome of response.text(), response.json(), etc, which seems to have to do with the fact that those functions are not actually wrapped, but instead most of the time resolve at the same time as the clone's blob in here.

To get it to work in our tests, we had to wrap those text etc functions in a waitForPromise, by overriding the fetch adapter like so:

WRAPPED_RESPONSE_FUNCTIONS.forEach((fn) => {
  let nativeFn = this[`_nativeResponse${fn}`] = Response.prototype[fn];

  Response.prototype[fn] = function(...args) {
    return waitForPromise(nativeFn.call(this, ...args));
  };
});

With both the native and polyfill support, I don't know the best way to fix this here + I don't know if there's any interest in fixing this in this repo in the first place. Any chance this can be picked up at some point?

@chriskrycho
Copy link
Contributor

I just saw this discussion and I could not possibly disagree more strongly with the idea that we should patch global fetch. I'm going to re-post here the comment I just left on the @ember/test-helpers repo where this came up, because it actually suggests a reason to keep around ember-fetch but with less silliness in its import APIs.


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.)

@chriskrycho
Copy link
Contributor

I'll add: I'm aware that this means you have to do one of two things when interacting with third-party libraries which themselves do a fetch or similar:

  • wrap their own top-level call in a test waiter
  • configure the fetch call they use

Most well-behaved data fetching libraries (e.g. Apollo) already allow you to do the second, so you could just pass them a wrapped fetch. In the case of a library which doesn't support that, you likely want to wrap whatever top-level calls they’re doing in a waiter (rather than all the sub-calls they might make) anyway—and again, that generalizes nicely beyond what the

Critically, in both cases, you're not invisibly changing the behavior of third-party libraries in ways they cannot anticipate.


Addendum: the fact that other frameworks do shenanigans like this is not itself a very good argument to me. Imports are great!

@ef4
Copy link
Contributor

ef4 commented Dec 1, 2022

Critically, in both cases, you're not invisibly changing the behavior of third-party libraries in ways they cannot anticipate.

We have a robust spec for fetch. I would not suggest replacing it unless we were going to provide a 100% spec-compliant replacement. There is no invisible change in behavior. If there's some reason we can't be 100% spec compliant I would change my mind, but I'm not aware of any limitation like that (because really we would be using the native implementation anyway, and merely observing what it's doing).

The disagreement here comes down to a philosophical question: I think our test suite environment is an ECMAScript host environment that should be free to implement any of the environment-provided features in any spec-compliant way that it wants. In the testing context, we have enough control to decide what constitutes "environment creation" vs "inside the tests". We can provide full-fidelity to the inside of the tests.

and there's no bright line between "replace fetch" and "replace Promise.then

There are at least two bright lines.

The first is what I already said, which is that monitoring all promises in a way that is both complete and spec-compliant is not nearly as easy as monitoring fetch. The spec for promises allows significantly more extensibility. Not all operations go through window.Promise or window.Promise.prototype.then.

The second is that, from a testing ergonomics perspective, monitoring every promise is not what you really want anyway. By analogy with tracked properties, where it's nicer to just track the roots of state and not try to track every intermediate result, the "roots of asynchrony" are operations like fetch, with dependent promises chained off them. Microtask scheduling makes it safe to ignore everything that's not these root operations (as long as you're flushing microtasks before moving onward).

@ef4
Copy link
Contributor

ef4 commented Dec 1, 2022

Also keep in mind we're talking only about test environments here. Where one of the most prevalent testing strategies in the community is already replacing fetch anyway!

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