use browser fetch if exists #63

xg-wang · 2017-08-14T21:23:05Z

GitHub/fetch expects a window that contains a fetch, Headers, Response... as the passed in self so it can skip the unnecessary polyfill. But in ember-fetch the passed in self is actually the exported object, which is {} at first.

xg-wang

https://github.com/github/fetch/blob/master/fetch.js#L8
Need these check otherwise these features can't be supported by github/fetch.

stefanpenner · 2017-08-16T05:38:48Z

This project explicitly forces the polyfil in all cases. This is because the polyfill is not complete, and switching between modes automatically is dangerous.

I would be open to a config setting to allow this as an opt-in.

xg-wang · 2017-08-19T03:19:16Z

@stefanpenner Just added the option which could be set in 'ember-cli-build.js' as

  var app = new EmberAddon(defaults, {
    'ember-fetch': {
      forcePolyfill: false
    }
  });

It defaults to true.

rwjblue · 2017-08-19T12:04:54Z

#45 is also somewhat related.

tchak · 2017-10-14T16:27:45Z

assets/browser-fetch.js.t

+      'Symbol',
+      'iterator',
+      'ArrayBuffer'
+    ];


I think if we are to add all of these we should also add fastboot polyfills in order to ensure parity.

Why are we adding FormData, FileReader, Blob to self? They are also not added to Fastboot fetch.

@tchak Just walked through PR list, these have been merged and are exported, but not in Fastboot...
#108 #93 #76

xg-wang · 2017-10-18T17:11:18Z

To ensure parity with Fastboot polyfills, supported properties(FormData, FileReader, Blob, etc) should only be used internally in github/fetch but not exported.
We previously export FormData, FileReader, Blob but they were actually missing in Fastboot.

stefanpenner · 2018-05-14T21:10:20Z

We explicitly don't do this right now, only because we don't want folks accidentally relying on non polyfilled features in development, and becoming surprised in production.

There is likely a strategy, were we detect these violations or minimize potential fallout. I would love for us to do so, so we can benefit from native fetch when possible.

@xg-wang thoughts?

xg-wang · 2018-05-15T05:57:38Z

@stefanpenner
I still think we should use native fetch if possible.
It is becoming stable and we should have it available for Ember developers and enable projects' support for fetch. There are also people who need the native fetch and won't have those edge cases on polyfilled browsers.
Guarding this new feature behind a flag as implemented in this PR is a good choice for now. This could be the starting point for us to gradually migrate.

The Github polyfill for fetch only take effect if window.fetch doesn't exist, IMO we should make ember-fetch behave similarly. As for now, we need to explicitly point out the difference in the readme (that ember-fetch never uses native fetch).

Make it more clear, the roadmap I would like for ember-fetch is

Explicitly point out this project discard native fetch and always use XHR next to the reference to Github polyfill
Finish this PR to provide a feature flag which needs to be explicitly turned on to provide access to native fetch - on par with the GitHub polyfill. When doing this we also need to check for testing so that pretender don't fail - and have this caveat highlighted in the readme to let people be aware.
Wait until Using Pretender with fetch. pretenderjs/pretender#60 close and correlated changes in mirage land, then drop the testing check mentioned above - this will ensure same behavior in prod and test
Default access to native fetch for a major version bump

We need to discuss the potential disparity between fastboot fetch and browser fetch, and where should changes be made. My assumption is that we can tolerate the disparity a little bit more since we didn't expect fastboot/browser behavior to be exactly same in else aspects.

@rwjblue I'm not sure about #45. We don't need to avoid import when targets have fetch since Github polyfill uses native fetch if exists.

stefanpenner · 2018-05-15T16:16:16Z

@xg-wang this sounds mostly reasonable. I really do want us to use fetch by default.

One key area that will need to still be dealt with is that IE11 (a supported platform) does not have fetch.

So for users that fall back to the polyfil (ie11 users), what features will they loose, and for those lost features what can we do to mitigate pain.

It's never fun to realize that some important aspect of your app is non-functional once you test in IE11.

tchak · 2018-05-15T16:26:13Z

@stefanpenner as far as I can tell, the main missing feature from polyfil is streams. I have not investigated it yet, but is it really unreasonable to polyfil by using progress events on XHR?

xg-wang · 2018-05-15T17:33:46Z

@stefanpenner - Github polyfill

is a polyfill that implements a subset of the standard Fetch specification

I will figure out the supplement set and see if we can do a warning message for those.

For native fetch it should be fine to leave it to the browser vendor behavior.

@tchak - Polyfillable or not, I think it's reasonable to let https://github.com/github/fetch decide.

stefanpenner · 2018-05-16T00:16:53Z

@tchak / @xg-wang thoughts on simply wrapping native fetch, in something that hides stream stuff by default? In a way that errors with a helpful message (and opt-opt instructions). Basically just catching the cases where the polyfill will fall short.

^^ would be for if we can't reasonably polyfil the remaining features.

xg-wang · 2018-05-16T03:56:15Z

We have these choices:

Forbid known experimental features available in a subset of targets and throw warnings
Allow access to experimental features and throw warnings, and offer ember-fetch APIs for checking availability. (This is something like this.get('fastboot.isFastBoot'))
Provide polyfill for experimental features based on option 2
Expose native fetch just as Github polyfill and do nothing

My thought:

These are also different levels of autonomy that we give to ember developers, and IMO should also take the browser support situation into consideration.
Experimental features like Streams and Priority hints (need a relatively more complete list though it may update frequently) are cool and people may want to try them out for available browsers.

My opinion:

I like option 2 but it takes some time to implement & figure out the complete list.
It would be reasonable and safe to start with option 1, and provide warnings for some features that we know people might be using "mistakenly".
I don't want to introduce more polyfills, would rather wait for native support.

@stefanpenner

tchak · 2018-05-16T09:06:32Z

The problem with leaving streams out is that it is the only way with fetch to show progress for uploads. See #99. Can we try to use https://github.com/creatorrr/web-streams-polyfill?

I feel that if we want for fetch to be a serious alternative to xhr it needs some level of feature parity.

stefanpenner · 2018-05-16T20:43:45Z

We have these choices:

We also have, in development/test only use polyfil, and in prod use real.

The problem with leaving streams out is that it is the only way with fetch to show progress for uploads. See #99. Can we try to use https://github.com/creatorrr/web-streams-polyfills

For users who want to use streams, we can allow a "alwaysUseNativeFetch" style option, which opts out of the above safety.

Thoughts?

stefanpenner · 2018-05-16T20:44:20Z

can we try to use https://github.com/creatorrr/web-streams-polyfills

I would love to here how well this works (tell me the downsides), and the impact of it.

xg-wang · 2018-05-17T07:17:20Z

I updated this patch so we can have some real code stuff to talk about.

xg-wang · 2018-05-17T15:08:44Z

index.js

@@ -75,15 +75,16 @@ module.exports = {
      } while (current.parent.parent && (current = current.parent));
    }

-    this.buildConfig = target.options['ember-fetch'] || {forcePolyfill: true};
+    this.buildConfig = target.options['ember-fetch'] || { preferNative: false };


Rename the option here as I personally feel it more comfortable for me.
This should also be force overwritten if in testing, will add this in next commit.

This sounds good. To help forks deal with the upgrade, could we detect if they use forcePolyfill property, and throw a useful error message?

Once released, this will also be a major bump.

I will squash the commits once I have a more meaningful implementation, right now it's more of a brainstorm

cibernox · 2018-05-17T19:52:22Z

While not technically related, one problem with using native fetch right now is that pretender and ember-cli-mirage won't work. It's not per-set a problem of this library, but will bite people if we go ahead and do this. Someone needs to make pretender work with fetch.

stefanpenner · 2018-05-17T19:57:19Z

@cibernox making pretender work with native fetch sounds good. I believe @xg-wang's proposal is to run the polyfill fetch during dev/tests, but native in production. This would allow pretender (used in dev) to "just work".

We should obviously follow up with a fetch solution for Pretender (pretenderjs/pretender#60). This shouldn't take much time, and I believe @xg-wang will be working on fetch + pretender compat next.

@cibernox thoughts?

(answered this comment with @xg-wang sitting next to me)

xg-wang · 2018-05-18T05:34:20Z

For this change, open the dummy app and see console to confirm it's working:

ember s: XHR finished loading: GET "http://localhost:4200/slow-data.json".
ember b --prod; http-server dist/: Fetch finished loading: GET "http://127.0.0.1:8080/slow-data.json".
ember test: Pass

Warn users like this:

 ❯ ember b --prod                                                                                          [22:33:24]
[ember-fetch] Prefer native fetch enabled, please be aware of browser and pretender support.

yarn link test on a new app works the same.

stefanpenner · 2018-05-24T15:46:33Z

tests/dummy/config/environment.js

@@ -45,6 +45,9 @@ module.exports = function(environment) {

  if (environment === 'production') {
    // here you can enable a production-specific feature
+    ENV['ember-fetch'] = {
+      preferNative: true


config/environment.js is for runtime configuration. But this is being used for build time configuration.

In general build time configuration should come from the ember-cli-build.js file. For example:

new EmberApp(trees, { 'ember-fetch': { preferNative: true } })

The intention was to let the user control the behavior under different environment, once pretender lands the fetch support ember-fetch no longer needs to consider different environment config. I'm going to move it back to the build config.
@stefanpenner

stefanpenner · 2018-05-24T15:46:50Z

index.js

-    browserTree = map(browserTree, (content) => `if (typeof FastBoot === 'undefined') { ${content} }`);
+    var preferNative = this.fetchConfig.preferNative;
+    browserTree = map(browserTree, (content) => `if (typeof FastBoot === 'undefined') {
+      var preferNative = ${preferNative};


also, should we call this preferNative or forceNative, which is more appropriate?

There will always be a fetch available, forceNative sounds like "if native is not available, do nothing".
So I was comparing preferNative and forcePolyfill and it turns out preferNative is more intuitive.

xg-wang · 2018-05-30T21:25:45Z

package.json

@@ -16,20 +16,23 @@
    "build": "ember build",
    "lint:js": "eslint ./*.js addon addon-test-support app config lib server test-support tests",
    "start": "ember server",
-    "test": "ember try:each"
+    "test": "npm run test:node && ember try:each",
+    "test:node": "mocha"


test:node might be confusing, since we are testing browser behavior in node env

xg-wang · 2018-05-30T21:26:37Z

package.json

    "ember-cli-babel": "^6.8.2",
    "node-fetch": "^2.0.0-alpha.9",
-    "yetch": "^0.0.1"
+    "whatwg-fetch": "https://github.com/github/fetch"


TODO, change this when new version released

xg-wang · 2018-05-30T21:30:16Z

tests/unit/mixins/adapter-fetch-test.js

-});
+    const fetchReturn = this.JSONAPIAdapter._ajaxRequest({});
+    // if in pretender with native fetch testing, pretender will polyfill native fetch
+    assert.ok(fetchReturn instanceof Promise || fetchReturn instanceof window.Promise);


Highlight
This is the only change in this file other than formatting and including tests in module. We have RSVP.Promise but pretender will have the Promise provided specified in ember-cli-pretender

xg-wang · 2018-05-30T22:49:00Z

Review needed @tchak @stefanpenner @rwjblue, should we mention stream and other fetch polyfill caveats?

If set to false, native `fetch` access will be enabled.

test on dummy app and 'yarn link'ed app

fetch should be called in the context of window

Use `Object.defineProperty` instead of property caching so future modification to window.<xxx> will also take effect. e.g. pretender swaps `window.fetch` will not work if ember-fetch cached the native fetch already. misc: fix misuse of Mergetrees, fix and reformat adapter test

xg-wang · 2018-07-14T05:23:40Z

Rebased on master. Investigating why fail Scenario ember-default and 2.12.

For fixing "Error: Assertion Failed: You have turned on testing mode, which disabled the run-loop's autorun. You will need to wrap any code with asynchronous side-effects in a run" in CI.

- node test remove ember-fetch.map - README mention github/fetch rather than Netflix/yetch - const/let over var

xg-wang · 2018-07-19T01:30:45Z

This PR is ready, need some review. @rwjblue @stefanpenner @tchak

stefanpenner

small tweaks, and then we are good.

stefanpenner · 2018-07-20T15:35:30Z

index.js

-  var expandedAbortcontrollerPath = expand(abortcontrollerPath, 'abortcontroller-polyfill-only.js');
-
-  var polyfillTree = concat(new MergeTrees([find(expandedFetchPath), find(expandedAbortcontrollerPath)]), {
+  const fetchTree = path.dirname(require.resolve('@xg-wang/whatwg-fetch'));


lets leave a comment above this line, briefly explaining why it is a fork and when we hope to get back onto the mainline.

Maybe we can actually have an issue on the fork with this info, and have the comment here cross link that?

Open 2 issues where the fork is being used, I will open another one under ember-fetch and cross link them.

pretenderjs/pretender#232
rwjblue/ember-cli-pretender#63

stefanpenner · 2018-07-20T15:36:06Z

test/prefer-native-test.js

+  describe(`Build browser assets with preferNative = ${flag}`, function() {
+    let output, subject, addon;
+
+    beforeEach(async function() {


we can drop async here, as it doesn't appear we have an await in the function

xg-wang · 2018-07-20T17:59:17Z

yarn registry seems not stable these days

CI passed on mine repo
https://travis-ci.com/xg-wang/ember-fetch/builds/79599780
https://ci.appveyor.com/project/xg-wang/ember-fetch-p6ymi/build/1

stefanpenner · 2018-07-20T22:14:23Z

released as v5.1.0 🎉🎉🎉🎉🎉🎉

xg-wang commented Aug 14, 2017

View reviewed changes

tchak reviewed Oct 14, 2017

View reviewed changes

xg-wang force-pushed the fix/browser-fetch branch from ef14409 to 492f9a4 Compare October 18, 2017 17:07

rwjblue mentioned this pull request Nov 5, 2017

refactor(jQuery): Remove $.ajax, use async/await, move custom paths t… miragejs/ember-cli-mirage#1206

Closed

st-h mentioned this pull request Jan 20, 2018

indicating progress #99

Closed

stefanpenner closed this May 14, 2018

stefanpenner reopened this May 14, 2018

xg-wang force-pushed the fix/browser-fetch branch from 492f9a4 to 42691b8 Compare May 17, 2018 07:16

xg-wang force-pushed the fix/browser-fetch branch from 42691b8 to ae7be04 Compare May 17, 2018 07:18

xg-wang commented May 17, 2018

View reviewed changes

xg-wang force-pushed the fix/browser-fetch branch from 07fee4e to 16f68ce Compare May 18, 2018 05:29

stefanpenner requested changes May 24, 2018

View reviewed changes

xg-wang commented May 30, 2018

View reviewed changes

xg-wang self-assigned this May 30, 2018

xg-wang added 10 commits July 13, 2018 22:14

Add option forcePolyfill

527fe56

If set to false, native `fetch` access will be enabled.

Add abortController to export prop, rename option

d822641

move preferNative to environment aware config

172adcf

test on dummy app and 'yarn link'ed app

read from build config

9215523

fix failed to execute 'fetch' on 'window'

ed1f0ce

fetch should be called in the context of window

Use node vm for unit test the actual vendor script

6ea7b3f

update readme for fetch related

5a9f791

update whatwg-fetch to @xg-wang/whatwg-fetch temporarily

0377f8c

update dependency

57fb401

xg-wang force-pushed the fix/browser-fetch branch from 6592741 to 57fb401 Compare July 14, 2018 05:22

xg-wang added 2 commits July 14, 2018 22:32

unloop in dummy app

cbe37fa

For fixing "Error: Assertion Failed: You have turned on testing mode, which disabled the run-loop's autorun. You will need to wrap any code with asynchronous side-effects in a run" in CI.

Changes after rebase

d79958f

- node test remove ember-fetch.map - README mention github/fetch rather than Netflix/yetch - const/let over var

xg-wang mentioned this pull request Jul 19, 2018

v5.0.0 has an error to include yetch-polyfill #118

Closed

xg-wang added 2 commits July 18, 2018 17:40

update ember-cli-pretender to enable fetch in pretender

f657cde

update deps

808bc50

stefanpenner reviewed Jul 20, 2018

View reviewed changes

xg-wang added 2 commits July 20, 2018 09:50

add comment to explain fetch fork

25f7273

drop unnecessary async in test

fdb5005

stefanpenner merged commit ed52bfd into ember-cli:master Jul 20, 2018

xg-wang deleted the fix/browser-fetch branch August 17, 2018 23:41

xg-wang mentioned this pull request Sep 7, 2018

Avoid import when targets support native fetch #45

Closed

use browser fetch if exists #63

use browser fetch if exists #63

Conversation

xg-wang commented Aug 14, 2017

xg-wang left a comment

Choose a reason for hiding this comment

stefanpenner commented Aug 16, 2017

xg-wang commented Aug 19, 2017 • edited Loading

rwjblue commented Aug 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xg-wang May 17, 2018 • edited Loading

Choose a reason for hiding this comment

xg-wang commented Oct 18, 2017

stefanpenner commented May 14, 2018 • edited Loading

xg-wang commented May 15, 2018 • edited Loading

stefanpenner commented May 15, 2018 • edited Loading

tchak commented May 15, 2018

xg-wang commented May 15, 2018 • edited Loading

stefanpenner commented May 16, 2018

xg-wang commented May 16, 2018 • edited Loading

tchak commented May 16, 2018

stefanpenner commented May 16, 2018

stefanpenner commented May 16, 2018

xg-wang commented May 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cibernox commented May 17, 2018

stefanpenner commented May 17, 2018 • edited Loading

xg-wang commented May 18, 2018 • edited Loading

stefanpenner May 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xg-wang May 30, 2018 • edited Loading

Choose a reason for hiding this comment

xg-wang commented May 30, 2018

xg-wang commented Jul 14, 2018

xg-wang commented Jul 19, 2018

stefanpenner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xg-wang commented Jul 20, 2018 • edited Loading

stefanpenner commented Jul 20, 2018

xg-wang commented Aug 19, 2017 •

edited

Loading

xg-wang May 17, 2018 •

edited

Loading

stefanpenner commented May 14, 2018 •

edited

Loading

xg-wang commented May 15, 2018 •

edited

Loading

stefanpenner commented May 15, 2018 •

edited

Loading

xg-wang commented May 15, 2018 •

edited

Loading

xg-wang commented May 16, 2018 •

edited

Loading

xg-wang commented May 17, 2018 •

edited

Loading

stefanpenner commented May 17, 2018 •

edited

Loading

xg-wang commented May 18, 2018 •

edited

Loading

stefanpenner May 24, 2018 •

edited

Loading

xg-wang May 30, 2018 •

edited

Loading

xg-wang commented Jul 20, 2018 •

edited

Loading