Embroider compat

[ef4]

Opening this issue because @xg-wang asked in Discord:

Hi @ef4 , do you know what should be changed for https://github.com/ember-cli/ember-fetch to be embroider compatible?

There are a couple different ways to answer.

First, does ember-fetch work today under @embroider/compat? Yes, as far as I can tell it works flawlessly, at least in the browser environment. We are still working through fastboot support issues in embroider, so I didn't test ember-fetch in the fastboot environment.

Second, how can ember-fetch be implemented as a native V2-formatted addon (assuming emberjs/rfcs#507 is accepted)?

I think it gets simpler to implement using the macro system. All the custom broccoli code becomes unnecessary. Here's a simplified sketch to show the basic idea:

// package.json
{
  ...
  "ember-addon": {
    "version": 2,
    "renamed-packages": {
      "ember-fetch": "fetch"
    },
    "build": "build.js"
  },

  // we have an optional peer dependency on ember-cli-fastboot
  // because we want to be able to check its configuration.
  "peerDependencies": {
    "ember-cli-fastboot": "*"
  },
  "peerDependenciesMeta": {
    "ember-cli-fastboot": {
      "optional": true
    }
  }
}

index.js would look something like:

import { macroCondition, getOwnConfig, getConfig, importSync } from '@ember/macros';

let implementation;

if (macroCondition(getConfig('ember-cli-fastboot').isFastboot)) {
  // we end up here if ember-cli-fastboot is installed and
  // if we're actually running inside fastboot. This assumes
  // that ember-cli-fastboot will expose an `isFastboot`
  // property to the macro system, but that is a safe assumption.
  // We can make sure it does so.
  implementation = importSync('node-fetch').default;
} else {
  if (macroCondition(getOwnConfig().needsPolyfill)) {
    // this is where you can put the shimming code
    // that currently lives in browser-fetch.js.t. I simplified
    // it here for brevity.

    // this library sets the global fetch
    importSync('whatwg-fetch');
  }
  implementation = fetch;
}
export default implementation;

and there would be a build.js that looks like:

export default class {
  configure() {
    return {
      needsPolyfill: checkIfPolyfillNeeded()
    }
  }
}

The only bit here that is unclear to me is that we may need to create an API for the addon to get the configured browser targets, so you can implement your checkIfPolyfillNeeded() function. That seems like a reasonable extension that can go into the next V2 package RFC (there will definitely be more, because we haven't accounted for all the things all addons need to do yet).

[ef4]

(As ember-fetch is already compatible, there's no need to keep this issue open unless you want to.)

[xg-wang]

Thank you @ef4 for the explanation!

[runspired]

So while it is true that ember-fetch can be consumed by an app using embroider, it's also still true that it needs substantial changes to be truly compatible with embroider, we should likely reopen this.

For EmberData, we currently are forced to do the following which is not compatible with us converting to a v2 addon.

import require, { has } from 'require';

let _fetch;
if (has("fetch")) {
  _fetch = require("fetch");
}

This is because ember-fetch manually splices define("fetch", into the build output vs relying on any other form of modules compilation.

[runspired]

this is the craziness that makes this not work with embroider v2 addons: https://github.com/ember-cli/ember-fetch/blob/master/index.js#L238-L269

#330 is highly related to this