Update to PureScript v0.15.0

[JordanMartinez]

Description of the change

Backlinking to purescript/purescript#4244

Migrates FFI to ES modules

---

Checklist:

• Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0000)")
• Linked any existing issues or proposals that this pull request should close
• Updated or added relevant documentation
• Added a test for the contribution (if applicable)

[JordanMartinez]

CI fails for now as I'm not entirely sure how we should update the FFI here.

[purefunctor]

This patch would make eslint pass and get tests to run as well.

diff --git a/.eslintrc.json b/.eslintrc.json
index 9d6a62b..d894fe7 100644
--- a/.eslintrc.json
+++ b/.eslintrc.json
@@ -1,6 +1,7 @@
 {
   "extends": "eslint:recommended",
   "parserOptions": { "ecmaVersion": 6, "sourceType": "module" },
+  "env": { "browser": "true", "node": true },
   "rules": {
     "block-scoped-var": "error",
     "consistent-return": "error",
diff --git a/packages.dhall b/packages.dhall
index 582d6d3..a7ee8b2 100644
--- a/packages.dhall
+++ b/packages.dhall
@@ -1,4 +1,5 @@
 let upstream =
       https://raw.githubusercontent.com/purescript/package-sets/prepare-0.15/src/packages.dhall
+        sha256:b7f601683fe3f760ea88f2375c2b08cfe43b98c9a4fc6d000ae8bc49ac8bfb07
 
 in  upstream
diff --git a/src/Affjax.js b/src/Affjax.js
index 318172a..5b011cf 100644
--- a/src/Affjax.js
+++ b/src/Affjax.js
@@ -1,6 +1,4 @@
-/* global XMLHttpRequest */
-/* global process */
-export function _ajax() {
+export function _ajax(timeoutErrorMessageIdent, requestFailedMessageIdent, mkHeader, options) {
   var platformSpecific = { };
   if (typeof module !== "undefined" && module.require && !(typeof process !== "undefined" && process.versions["electron"])) {
     // We are on node.js
@@ -39,56 +37,54 @@ export function _ajax() {
     };
   }
 
-  return function (timeoutErrorMessageIdent, requestFailedMessageIdent, mkHeader, options) {
-    return function (errback, callback) {
-      var xhr = platformSpecific.newXHR();
-      var fixedUrl = platformSpecific.fixupUrl(options.url, xhr);
-      xhr.open(options.method || "GET", fixedUrl, true, options.username, options.password);
-      if (options.headers) {
-        try {
-          // eslint-disable-next-line no-eq-null,eqeqeq
-          for (var i = 0, header; (header = options.headers[i]) != null; i++) {
-            xhr.setRequestHeader(header.field, header.value);
-          }
-        } catch (e) {
-          errback(e);
+  return function (errback, callback) {
+    var xhr = platformSpecific.newXHR();
+    var fixedUrl = platformSpecific.fixupUrl(options.url, xhr);
+    xhr.open(options.method || "GET", fixedUrl, true, options.username, options.password);
+    if (options.headers) {
+      try {
+        // eslint-disable-next-line no-eq-null,eqeqeq
+        for (var i = 0, header; (header = options.headers[i]) != null; i++) {
+          xhr.setRequestHeader(header.field, header.value);
         }
+      } catch (e) {
+        errback(e);
       }
-      var onerror = function (msgIdent) {
-        return function () {
-          errback(new Error(msgIdent));
-        };
+    }
+    var onerror = function (msgIdent) {
+      return function () {
+        errback(new Error(msgIdent));
       };
-      xhr.onerror = onerror(requestFailedMessageIdent);
-      xhr.ontimeout = onerror(timeoutErrorMessageIdent);
-      xhr.onload = function () {
-        callback({
-          status: xhr.status,
-          statusText: xhr.statusText,
-          headers: xhr.getAllResponseHeaders().split("\r\n")
-            .filter(function (header) {
-              return header.length > 0;
-            })
-            .map(function (header) {
-              var i = header.indexOf(":");
-              return mkHeader(header.substring(0, i))(header.substring(i + 2));
-            }),
-          body: platformSpecific.getResponse(xhr)
-        });
-      };
-      xhr.responseType = options.responseType;
-      xhr.withCredentials = options.withCredentials;
-      xhr.timeout = options.timeout;
-      xhr.send(options.content);
+    };
+    xhr.onerror = onerror(requestFailedMessageIdent);
+    xhr.ontimeout = onerror(timeoutErrorMessageIdent);
+    xhr.onload = function () {
+      callback({
+        status: xhr.status,
+        statusText: xhr.statusText,
+        headers: xhr.getAllResponseHeaders().split("\r\n")
+          .filter(function (header) {
+            return header.length > 0;
+          })
+          .map(function (header) {
+            var i = header.indexOf(":");
+            return mkHeader(header.substring(0, i))(header.substring(i + 2));
+          }),
+        body: platformSpecific.getResponse(xhr)
+      });
+    };
+    xhr.responseType = options.responseType;
+    xhr.withCredentials = options.withCredentials;
+    xhr.timeout = options.timeout;
+    xhr.send(options.content);
 
-      return function (error, cancelErrback, cancelCallback) {
-        try {
-          xhr.abort();
-        } catch (e) {
-          return cancelErrback(e);
-        }
-        return cancelCallback();
-      };
+    return function (error, cancelErrback, cancelCallback) {
+      try {
+        xhr.abort();
+      } catch (e) {
+        return cancelErrback(e);
+      }
+      return cancelCallback();
     };
   };
-}();
+}
\ No newline at end of file
diff --git a/test/Main.js b/test/Main.js
index 5813d41..b0bfa42 100644
--- a/test/Main.js
+++ b/test/Main.js
@@ -1,3 +1,10 @@
+import express from "express";
+import bodyParser from "body-parser";
+import { dirname } from 'path';
+import { fileURLToPath } from 'url';
+
+const __dirname = dirname(fileURLToPath(import.meta.url));
+
 export function logAny(a) {
   return function () {
     console.log(a);
@@ -6,9 +13,7 @@ export function logAny(a) {
 }
 
 export function startServer(errback, callback) {
-  var express = require('express');
   var app = express();
-  var bodyParser = require('body-parser');
 
   // Always make req.body available as a String
   app.use(bodyParser.text(function() { return true; }));

[garyb]

I'm not sure this library can be fixed "properly" for ES modules while supporting both the browser and workarounds for node/electron/etc. I've been wondering for a while if this library's time has come, since there are competing sets of demands for what it should support, and things that don't fit well into the current model (progress events, for example). And there's purescript-web-fetch and purescript-web-xhr bindings now which didn't exist at the time this library was written.

An option that might be worth considering if we do want to keep it going, is to make this library implementation agnostic by having the functions accept a kind of "driver" value, and then create purescript-affjax-web and purescript-affjax-xhr2 libraries to provide the driver that suits the platform the code is intended for. That would avoid the need for any kind of bundling workarounds and all that, because you'd only include the one you need in the project.

[JordanMartinez]

An option that might be worth considering if we do want to keep it going, is to make this library implementation agnostic by having the functions accept a kind of "driver" value, and then create purescript-affjax-web and purescript-affjax-xhr2 libraries to provide the driver that suits the platform the code is intended for. That would avoid the need for any kind of bundling workarounds and all that, because you'd only include the one you need in the project.

How would the dependencies play out? There are technically two options:

[garyb]

The first of the two options there - the idea behind what I said is that you only end up with the correct driver being included in the project, avoiding the need for any bundling-related workarounds.

[JordanMartinez]

The first of the two options there - the idea behind what I said is that you only end up with the correct driver being included in the project, avoiding the need for any bundling-related workarounds.

Ok, that works. Now how do we create new repositories? That's a gap in my knowledge.

[thomashoneyman]

You can just initialize a new repository with Spago, then run the contrib updater tool. Docs here:

https://github.com/purescript-contrib/governance/blob/main/updater/docs/01-Generate.md

We may need to update the template files so that CI includes purs-tidy and purescript: "unstable".

[garyb]

I'm having second thoughts about this, 😄 as it is a fairly large change.

Basically, I want to resolve/avoid #161, and especially post-ESM I'm not sure how best to resolve it. It seems esbuild makes no attempt to resolve dynamic imports, so it would "just work" if we did something with that, but... yeah, I dunno.

[JordanMartinez]

@garyb We discussed this repo's issue with ES modules in today's working group call.

Below is an example for how the code is currently written:

import Affjax as AX
import Affjax.RequestFormat as ResponseFormat

foo = do
  result1 <- AX.get ResponseFormat.json "/api/one"
  result2 <- AX.get ResponseFormat.json "/api/two"
  result3 <- AX.get ResponseFormat.json "/api/three"

Here is my understanding of Option 1. Option 1 means everyone would "consume" the affjax-core library, but have to supply the driver via affjax-web or affjax-node (or affjax-electron). If used, then everyone would need to update their code to pass a driver arg into the affjax calls. For example:

import Affjax as AX
import Affjax.Driver.Node (nodeDriver)
import Affjax.RequestFormat as ResponseFormat

foo = do
  result1 <- AX.get nodeDriver ResponseFormat.json "/api/one"
  result2 <- AX.get nodeDriver ResponseFormat.json "/api/two"
  result3 <- AX.get nodeDriver ResponseFormat.json "/api/three"

Here's my understanding of Option 2. In option 2, end-users do not "consume" the affjax-core library. Instead, they "consume" the driver libraries (e.g. affjax-node, etc.), and update their imports and aliases for modules.

- import Affjax as AX
import Affjax.RequestFormat as AXRF
+ import Affjax.Node as AXN

foo = do
  result1 <- AXN.get ResponseFormat.json "/api/one"
  result2 <- AXN.get ResponseFormat.json "/api/two"
  result3 <- AXN.get ResponseFormat.json "/api/three"

AFAIU, there isn't really a good way to make Affjax work on multiple environments while using ES modules. If my understanding between the two is correct, the second seems to entail less breakage than the first.

What are your thoughts? Since you're the primary maintainer of this library, your decision stands. Whatever it is, I'd like to work on it so we can unblock the ecosystem update.

[garyb]

I think both cases are much the same actually - to provide request, get, post etc. from each of the driver libraries I imagine they'd just be applying the driver defined in the library to the request, get, post, etc. functions defined in affjax-core, knocking off the driver argument.

The only downside of including these functions per-driver is that it's more stuff to keep in sync if we introduce a new function in core, and similarly we should reject changes to the per-driver libraries that aren't based on a partially-applied-re-export from core. Since affjax is basically done in terms of interface I think that's fine though, and I agree it's less breaking if people can just change one import and dependency and have things work after this change.

[JordanMartinez]

Ok, this PR and the other two are ready for review. Once this PR is approved and merged, I can add affjax back to the package set, then update those two other PRs' CI files, then merge those PRs, and lastly add them to the package set.

• Add Node driver purescript-affjax-node#1
• Add web driver purescript-affjax-web#1

[JordanMartinez]

Also, I moved the request code out of Affjax and into Affjax.Driver. This makes the compiler errors more understandable. Rather than errors like, "could not unify type AffjaxDriver with type { .... }", one will be notified that Affjax no longer exports request, making it more obvious that one needs to use a different module.

[JordanMartinez]

Also, I didn't include the response field in the AffjaxDriver since both the Node and web versions implemented it via return xhr.response;.

[garyb]

How should the web driver's tests be run? Before this split, Node could be used to verify the web code worked properly.

Re-implementing the xhr2 driver in test code is one option, but it kinda sucks. The other would to be to do some kind of browser automated testing thing, but that's probably even worse since it takes constant maintenance to keep browser-based testing working due to browser version upgrades and corresponding webdriver version bumps.

We could just leave it without automated tests, as realistically the implementation there should be trivial, and unchanging after initial implementation, since it's just passing through the global object, and the rest of Affjax's FFI code will be exercised by the node driver tests.

I don't really think there is a perfect solution. 😕

[garyb]

I'll take a look at the rest of the changes here later, probably tomorrow. Thanks for sorting all this out!

[thomashoneyman]

I'll wait to add the packages to the registry until we know this is the direction we're going — once @garyb has had a chance to review. Thanks!

[JordanMartinez]

Sounds good!

[garyb]

I see what you're going for with moving the stuff out of Affjax into Affjax.Driver, but I think the naming of that is a little strange, since most of the module's content is not driver interface or implementation really. I'd suggest we just keep everything in Affjax.

Actually, we should probably re-export Error/printError/etc from the new Affjax.Driver.* modules too, so they can be a complete drop in for the Affjax module when migrating existing code.

[garyb]

I don't think we need this Xhr type, since it's not actually used anywhere aside from within this comment?

[JordanMartinez]

This is ready for another review.

[garyb]

Looks great 👌, thanks again for taking this on

[thomashoneyman]

@JordanMartinez Does the README still need to be updated to clarify how to use Affjax on Node or the browser now?

[JordanMartinez]

@thomashoneyman Ah, yeah it does.