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

Rename import path to ember-fetch #330

Open
chriskrycho opened this issue Jun 28, 2019 · 11 comments
Open

Rename import path to ember-fetch #330

chriskrycho opened this issue Jun 28, 2019 · 11 comments

Comments

@chriskrycho
Copy link
Contributor

chriskrycho commented Jun 28, 2019

It's a best practice in general, for imports to come from their package name rather than some more "convenient" name, and will become more so as Embroider lands:

New addons should not use renaming rules because it's confusing when the imports people type don't align with their real dependencies.

The import style here should simply be:

import fetch from 'ember-fetch';

This is not a blocker for use with Embroider, as there are solutions to support the existing flow, but is a nice improvement and will eliminate one thing that has to be done for this to use Embroider. It also has the nice upside of eliminating all shenanigans required to have this do the right thing for TypeScript integration!

This would be a breaking change, but should also be trivially codemod-able.

Copy link
Member

rwjblue commented Jun 28, 2019

We could start by providing both modules, and deprecate the fetch one. Wouldn't need to be a breaking change until we drop support (which could happen "far in the future").

@chriskrycho
Copy link
Contributor Author

Yeah, I like that as a path forward.

@ef4
Copy link
Contributor

ef4 commented Jul 2, 2019

Given that global fetch is a stable WHATWG standard, what do we gain by keeping an importable ember-specific package, as opposed to polyfilling the global fetch in each environment we care about (just fastboot and IE11, I think) and declaring that all Ember apps and addons are free to use global fetch?

Implementation-wise, the two are equally interceptable.

@chriskrycho
Copy link
Contributor Author

Seems perfectly reasonable to me. We’d just want a nice configurable switch for that if it’s ending up in the vendor bundle, or (better?) to deliver it separately.

Copy link
Member

rwjblue commented Jul 2, 2019

I think thats fine in general, it would also allow easier "use native" (since we would literally do nothing if your targets all have fetch).

We'd have to confirm that its "fine" in SSR environments too (since there is not a fetch global there), it would add to the list of expected globals in the fastboot sandbox setup (which is probably fine, but worth mentioning).

@stefanpenner
Copy link
Collaborator

Given that global fetch is a stable WHATWG standard, what do we gain by keeping an importable ember-specific package, as opposed to polyfilling the global fetch in each environment we care about (just fastboot and IE11, I think) and declaring that all Ember apps and addons are free to use global fetch?

@ef4 Im perfectly good with this, I prefer that approach.

Questions:

  • In this world, does ember-fetch provide said polyfill or something else
  • Currently import fetch from 'fetch'; is always the polyfill, to ensure that 100% compatibility between ie11. This is clearly not ideal, but the alternatives come with some risks. My thought was, once i11 is off our radar we re-eval
  • Currently the polyfill version also provides support for libraries such as pretender, which tap into XMLHttpRequest via FakeXMLHttpRequest(which is what the polyfil uses). This was done out of pragmatism, and I suspect alternative approaches could be taken.

@BarryThePenguin
Copy link
Contributor

BarryThePenguin commented May 26, 2020

Forcing the use of import fetch from 'fetch'; prevents Ember apps from using other npm packages that rely on the global fetch API, along with Response, Request, Headers, etc..

This isn't just an issue for IE11, where ember-fetch doesn't polyfill the fetch global..

It's also an issue for libraries that expect the native Response object. For example..

import { Response } `fetch`;

console.log(Response) // Response() { [polyfilled code] }
console.log(window.Response) // Response() { [native code] }
console.log(Response instanceof window.Response) // false

Another example is Rollbar, which tracks events for Telemetry, in part by wrapping the global xhr and fetch APIs

@chriskrycho
Copy link
Contributor Author

Following up on this (as I'm poking at some related things):

  • If we want a "ponyfill" that supplies native if it exists or a polyfill if not, that's fine, but it should be from ember-fetch instead of fetch as the module import, for the reasons articulated a couple years ago.
  • But also: this whole package is mostly irrelevant in a post-🔥 DIAF IE11 🔥 world, and should probably just be put into "maintenance/critical bug fixes only" mode once Ember 4 comes out.

@xg-wang
Copy link
Member

xg-wang commented Apr 24, 2021

ember-fetch also polyfills AbortController which isn't available on Safari until 12.1. https://caniuse.com/?search=abortcontroller the support has good coverage now.

IMO in a post (IE11 / Safari12) world, we should just move the fastboot support part into fastboot its own (maybe think about how we expose the isomorphic fetch API a bit more), and not rely on polyfill in the browser at all.

@ef4
Copy link
Contributor

ef4 commented Jan 31, 2022

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Oct 3, 2023

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

7 participants