clearly doc requirements for lib use

[jgravois]

• fetch polyfill
• user endpoints need to support CORS
• browser/node versions
• ?

[patrickarlt]

Before we do this we need need to figure out how and if we want to bundle dependencies.

How this currently works in that @esri/rest-request requires you install isomorphic-fetch, isomorphic-form-data and es6-promise as peer dependencies. In general this means that if you already require these in you project there will be no duplication. No matter how you import or require @esri/rest-* in your project you will be required to resolve those dependencies either by installing with NPM, inserting the right script tags or mapping the paths in your AMD config. In general I think this works well but it is a little confusing since you have to install everything as peer dependencies ect. This means users MUST use the polyfills we specify at the versions we specify and their bundlers will discover and include the dependencies automatically.

The other way we could do this is like this:

1. Don't import any polyfills. Just rely on the globals to be there.
2. Don't list anything as peer dependencies.
3. Instruct users that the will need to install a polyfill for fetch, Promise and FormData (node only) and make recommendations.

This method moves the responsibility to polyfill to the users application. This means a little more setup time and a little more managing of dependencies for the user, but it means that they can bring their own fetch, Promise and FormData libraries depending on their application.

---

This also gets me thinking about something @ajturner mentioned. Should we allow passing in a different fetch implementation as long as it meets our requirements? This would eliminate the need for requiring a fetch polyfill and simply require users to implement the following interfaces:

export interface IFetchableOptions {
  body: FormData | string;
  method: HTTPMethods;
}

export interface IFetchableResponse {
  json(): Promise<object>;
  text(): Promise<string>;
  blob(): Promise<Blob>;
}

export type IFetchable = (
  url: string,
  options: IFetchableOptions
) => Promise<IFetchableResponse>;

This is the subset of the fetch spec we actually use. For example In Angular you could wrap this as follows:

// import angular deps
import { Injectable } from '@angular/core';
import { Http } from '@angular/http';

// import toPromise so we can convert the observable into a Promise to match
// the fetch spec
import 'rxjs/add/operator/toPromise';

// import IFetchable so can typecheck the implimentation
import { IFetchable } from "@esri/rest-request";

// export a service that provides a `fetch` implimentation powered by
// Angulars HTTP module
@Injectable()
export class FetchService {
  constructor(private http: Http) { }

  get fetch () : IFetchable {
    return function (url, options) {
      return this.http.request(url, options).toPromise();
    }
  }
}

// now in some other file we can get our FetchService and pass its fetch into
// the requests like we do for auth.
import { getItems } from "@esri/rest-portal";

@Injectable()
export class FetchService {
  constructor(private fetch: FetchService) { }

  getItems () {
    // now we just pass our fetch implementation down the underlying @esri/rest-request.
    return getItems(username, {}, {
      authentication: session,
      fetch: fetch.fetch
    })
  }
}

[jgravois]

sounds like there's no easy way to ship something straightforward, self-contained and ready to use and simultaneously leave a backdoor to allow folks to substitute comparable dependencies from their framework or alternate lib of choice.

i don't have a strong opinion about this but my gut instinct is to prioritize the first use case and leave more advanced devs to their own devices.

real user feedback here would be extremely helpful.

[patrickarlt]

I'll be so bold as to make a few assumptions:

1. If you are using a bundler (webpack, rollup, browserify) and NPM seeing the required peer dependencies and installing them and getting everything configured won't be that difficult as we want to give samples and guides for each it should be really easy.
2. If you are not using a bundler it might make sense to also ship a version of @esri/rest-request that ALSO bundles in the dependencies so if you are using AMD or using it via the CDN you have use the bundled version (which includes all deps) or the unbundled one (bring your own deps) but that said configuring the dependencies shouldn't be that difficult.

[tomwayson]

I'm strongly in favor of not having dependencies (peer or otherwise) on any fetch/promise libraries. I think catering to the lowest common denominator ends up optimizing for legacy and penalizing everyone else.

Instead I prefer @patrickarlt's proposal of relying on consumers to bring their own polyfill (yes it's that kind of party) if they need to support scenarios where built-in fetch isn't available. I looked into this kind of thing for promisifying esri-loader, and I got good suggestion on SO that proposes to go a step further and polyfill the built-ins at runtime if they are missing. I'd say that's reasonable in an app, but not the job of a library. Generally speaking, I'm in favor of optimising for evergreen browsers and pushing the burden of supporting legacy browsers to the specific scenarios where it's required.

I also think it's really important to let consumers provide a specific implementation of IFetchable if, for example, they want to use their framework's fetch implementation as @patrickarlt demonstrates above. In that example, I assume that if the fetch option is not provided that it falls back to the built-in global. And if the user is using IE11, or the consuming app developer wants to support such a user, then they get what they deserve.

[patrickarlt]

@tomwayson @jgravois I'm fine with dropping peer dependencies and just making sure the globals are available. We probably just need to advise people to so the following:

1. Make sure they fetch polyfilled and available globally for older Safari, IE 11, iOS and older Android browsers https://caniuse.com/#search=fetch
2. Make sure Promise is polyfilled and globally available for IE 11 and older Android browsers
3. At least a full ES 5 environment which should not need polyfilling on most browsers https://caniuse.com/#search=es5

Dynamically importing polyfills is something I never really considered but I really like the idea of it because it makes using the library a no brainer. If we do do this we just have to note that we will import the polyfills from a CDN (unpkg? cdnjs?) and modify the global scope if dependencies do not exist. If users want to use specific polyfills or versions of polyfills they just have to load them before they make their first request. I agree with @tomwayson this this might fit better in an app but it makes using the library so easy that I still think we should do it.

I"m going to work on the following:

• Remove peer dependencies.
• Dynamically load Promise and fetch dependencies via <script> tag (browser) or require (Node) if they are not present on the global scope.
• Write up the relevant info in a guide.

[tomwayson]

I'd still prefer to leave the dynamic polyfilling to apps, maybe we could just include a sample app that does it, and/or a detailed doc?

That said, if you're going to do it, the ideas in this article really resonated w/ me. Looks like polyfill.io is the way to go.

[patrickarlt]

Leaving this here for future reference https://cdn.polyfill.io/v2/polyfill.min.js?features=fetch,Promise&flags=always

[patrickarlt]

@tomwayson @jgravois, I'm at a bit of a sticky point here...

I have automatic polyfilling working in browsers. In browsers lacking fetch or Promise at runtime it will automatically loaded from polyfill.io.

The sticky part is what the behavior should be in Node.js. You cannot do this:

if (isNode) {
  require('isomorphic-fetch');
} else {
  // load from polyfill.io
}

This means that Webpack/Rollup/Browserify will read the require statement and include the code at that point. We could disable this by instructing people to add isomorphic-fetch, isomorphic-form-data and es6-promise to their externals when using Webpack or Rollup. The other alternative would be to throw an error in Node is we can't find the dependencies and instruct users to install the packages.

I'm more in favor of throwing the error.

[jgravois]

sounds like there's no easy way to ship something straightforward, self-contained and ready to use and simultaneously leave a backdoor to allow folks to substitute comparable dependencies from their framework or alternate lib of choice.

unless you're @patrickarlt. 😎

i think having a plug and play browser lib that polyfills only when necessary and gives advanced developers an opportunity to roll their own fetch is really awesome!

and call it a double standard, but i never gave any thought to node developers in this regard because dependency management is so much less of a hassle when bundlers/frameworks aren't part of the equation.

if we have doc that outlines the node specific dependencies and ship an example, just throwing an informative error seems totally reasonable to me.

[patrickarlt]

I never looped back and updated this issue. Right now we are simply relying on globals that we expect users to install but we don't throw any errors or try anything fancy if they are not there or polyfills aren't loaded.

I could try to make the following work:

1. If request is called and we are in a browser environment (as far as we can tell) and we do not have the necessary globals, try to load the polyfills for the globals and try again.
2. If request is called in Node and the required globals are not found throw an error and try again.

I'm also ok with just throwing an error that links to the get stared guide if we cant find the globals.

[tomwayson]

I'm also ok with just throwing an error that links to the get stared guide if we cant find the globals.

Works for me. We can always add the fancy stuff later.

[patrickarlt]

@tomwayson @jgravois ok then this probably needs to be a v1 issue then. I'll wrap this up tomorrow.

[tomwayson]

FYI - here's what I ended up doing for esri-loader:

https://github.com/Esri/esri-loader#promises