Archive this repo?

[sandstrom]

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]

Would this not still be useful because of the imports?

[sandstrom]

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

[mansona]

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

[kboucher]

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]

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]

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]

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]

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

[ef4]

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]

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]

Yes, that sounds good to me. So to roll this out we need to 1. Do the feature in ember/test-helpers to make it hook into global fetch. (This work must be fastboot-compatible too). 2. Do a feature in fastboot that provides a fetch global on node versions that don’t have it. This can be in the actual “fastboot” package. It can use the same polyfill that ember-fetch was using. 3. Archive ember-fetch with clear instructions on deleting the imports and upgrading ember/test-helpers and fastboot. 4. Update the ember app blueprint to remove ember-fetch.

…

On Thu, Jun 30, 2022 at 1:30 PM Simon Ihmig ***@***.***> wrote: 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 <https://github.com/ember-cli/ember-fetch/blob/master/assets/browser-fetch.js.t#L55-L65> Given that the addon - uses so much broccoli magic (just look at index.js <https://github.com/ember-cli/ember-fetch/blob/master/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() <https://github.com/emberjs/ember-test-helpers/blob/master/addon-test-support/%40ember/test-helpers/settled.ts> 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. — Reply to this email directly, view it on GitHub <#656 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACN6MWNZMHXLJQN2SYWAVTVRXKVBANCNFSM5DUE7CLQ> . You are receiving this because you commented.Message ID: ***@***.***>

[robinborst95]

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?