jorgebucaran / hyperapp

1kB-ish JavaScript framework for building hypertext applications
MIT License
19.08k stars 780 forks source link

API for fetch-effect #877

Closed zaceno closed 3 years ago

zaceno commented 5 years ago

In my humble opinion, there needs to be an official fetch effect (or http, or ajax or whatever) for the final release of Hyperapp 2. Otherwise, it will be the first thing people ask for, and telling them to make their own would be inconsistent with the message that "you should not need to write your own effects most of the time".

The question is only what the API should be. I opened this issue to have that discussion, as it isn't as easy as one might think at first.

My first suggestion would be something like this:

Like:

Fetch({
  url: ... 
    //  the URL in a native fetch(url) call, mandatory

  options: {...}, 
  //the options in a fetch(url, options) call. 
  // Optional, default: undefined or {},

  onResponse: ... ,
  // fetch(...).then(resp => dispatch(props.onResponse, resp)
  // Optional, default: no dispatch

  onFailure: ...,
  //fetch(...).catch(err => dispatch(props.onFailure, err)
  // Optional, default: no dispatch

  bodyType: ...
  //only relevant if using onBody callback
  // can be 'text', 'json', 'blob', 'formData', 'arrayBuffer'
  // Optional, default 'text',

  onBody: ...
  // fetch(...)
  //    .then(response => response[props.bodyType]())
  //    .then(body => dispatch(props.onBody, body)
  // Optional, default: no dispatch

  onErrorParsingBody: ...
  // fetch(...)
  //    .then(response => response[props.bodyType]())
  //    .catch(error => dispatch(props.onErrorParsingBody, error)
  // Optional. Default: no dispatch
})

The thing that concerns me with this, is that fetch will resolve for any response from the remote. Including 404's et c. That means you'll get onBody callbacks even when there is no body, or the body is not of the bodyType you specified (in which case onErrorParsingBody will be dispatched).

So I was thinking, we could limit the body-parsing routine to be done only for 200 OK responses, and only if the request was using the GET method. But perhaps that's to restrictive?

If we limit body parsing to GET requests with 200 OK responses, then we'll need another option: onNotOK (maybe?) for the case when we got a response but decided not to parse the body. Or is that getting too complicated?

zaceno commented 5 years ago

Clearly the complicating issue here is that what we most often want is the body (most often parsed JSON). And that requires a two-step promise-chain, with some logic in between.

A more "close-to-native" approach would be to have simply:

Http.fetch({
  url: ...,
  options: {...},
  onResolve: GotResponse,
  onReject: NetworkIssue
})

But then we'd need a secondary effect returned from GotResponse. Perhaps like:

const GotResponse = (state, response) => {
   if (response.statusCode !== 200) return state
   return [state, Http.BodyAsJSON({
      body: response.body,
      onResolve: GotData
   })]
}

I think people will dislike having to make intermediate actions like this. But perhaps it's actually the only sane approach.

johnkazer commented 5 years ago

Statechart option? https://statecharts.github.io/xstate-viz/ (general case) https://xstate.js.org/docs/packages/xstate-react/#configuring-machines (a React example, could be modified slightly for Hyperapp)

https://blog.usejournal.com/handling-data-fetching-with-state-machines-4e25b6366d9 is a first-principles state-machine approach using React

zaceno commented 5 years ago

@johnkazer

I'm not completely sure what you're suggesting. If it is that we make an effect for fetch which has a dependency on xstate, that's not an option. Hyperapp is dependency free today and I don't think anyone is interested in changing that.

Your state chart does illustrate something very clearly: that fetch is a multi-step process where users will want to configure it in many different ways.

Hence I completely reject my first proposal and suggest this:

/*
Equivalent to:
fetch(url, options)
  .then(response => dispatch(onResolve, response)
  .catch(error => dispatch(onReject, error)
*/
Http.Fetch({url, options, onResolve, onReject})

/*
Equivalent to:
body[type]()
  .then(data => dispatch(onResolve, data))
  .catch(error => dispatch(onReject, error))
*/
Http.ParseBody({body, type, onResolve, onReject})

There's also the possibility of integrating AbortController signals, and progress tracking, but I think the above will suffice for most use cases.

zaceno commented 5 years ago

@okwolf Your hyperapp-fx lib has a fetch/http effect right? What are your thoughts?

johnkazer commented 5 years ago

The latter option for a state-machine approach doesn't have a dependency - just implements a simple comparable approach.

Either way, I think the point you raise about clear steps is a good one.

zaceno commented 5 years ago

@johnkazer Ah, I think I see what you're saying: by splitting up the flow as several effects rather than one huge one with various optional callbacks, we allow users more freedom in designing their desired flow, using state-charts for example. That makes sense, and is basically what I took away from your links, why I now believe more in having an effect for doing the fetch request, and then a collection of effects for managing the result of it (body-parsing, abort-signalling, progress-tracking)

SkaterDad commented 5 years ago

Any interest in basing this off XMLHttpRequest, rather than fetch? Since you're abstracting over it anyway, the awkward API isn't a big deal.

Pros: Browser support w/ easy cancellation (without polyfills), progress tracking of uploads, maybe more?

On an API-design note, I like how wretch handles errors. The library sorts out the various types of errors, and you can catch them as you like. https://github.com/elbywan/wretch#catchers

okwolf commented 5 years ago

Seems pretty similar to { Http } from "hyperapp-fx" except for different action and body type props. I'm noticing a trend toward dropping support for browsers without fetch, but if you really need it, there are polyfills.

sergey-shpak commented 5 years ago

@SkaterDad no worries about browser support, - there is no easy way to cancel hyperapp#2 'fetch' effect in general, you have to treat 'fetch' as a subscription to make it cancelable )

zaceno commented 5 years ago

@okwolf I checked the internals of your Http effect now (which I hadn't earlier) and see you're doing what I suggested about only parsing the body if the status code is 200. But you have a different take on errors. You use the same error callback when:

Is that correct?

I'm sure in the long run, people will want more logic branches, and ability to use signals, tracking (for downloads and uploads ). But for now, just to give people something, perhap's @okwolf's design is good enough? What can't you do with it?

If the the things you can't do with it are uncommon enough, we can still tell people to make their own fx/subs. We just shouldn't have to tell them that for things that almost everyone needs.

So now I'm leaning more towards something even simpler than what I first suggested: follow @okwolf's approach (https://github.com/okwolf/hyperapp-fx/blob/498416ad98d713b3d1a5a5d2a74a6335165ad7bb/src/fx/Http.js#L3).

But only for now. For later it will need to be more elaborate I'm quite sure.

okwolf commented 5 years ago

@zaceno your summary sounds accurate. My HTTP effect is built for users who for the most part care if a request succeeded or failed overall. The downside to this is if you need to react to different errors in different ways, your error action will need conditional logic based on inspecting the payload.

zaceno commented 5 years ago

Another question (which @SkaterDad wisely brought up) is: fetch or XmlHttpRequest ? Yes fetch can be polyfilled, but Hyperapp itself is compatible with IE >= 10 (I think), so should we make sure all fx and subs are as well? (without polyfilling) - If so: xhr it is.

okwolf commented 5 years ago

@zaceno for 3rd party I still feel fine with using fetch, but you do bring up a good point about what to use for the official @hyperapp/http library.

Ultimately it's an individual judgement call how many of your users are in the remaining small percentage without fetch support.

zaceno commented 5 years ago

Yeah, I suppose so. Personally I'd be fine dropping IE support all together in v2. But even if core remains IE-compatible, it could be argued that IE is a special use case so they have to write their own effects. I guess it's a question for @jorgebucaran

okwolf commented 5 years ago

I would rather not base the HTTP effect API on the awkward XmlHttpRequest implementation, and if the options are instead based on those from fetch, why not use it?

Just to be clear, what I'm advocating for isn't that users requiring IE support write their own FX, but that they have to bring their own polyfill for this feature. create-react-app ships with these polyfills as a reference.

zaceno commented 5 years ago

@okwolf

what to use for the official @hyperapp/random library.

Is there something in there that doesn't work with IE10? I didn't notice anything, but I only skimmed through quickly.

okwolf commented 5 years ago

Is there something in there that doesn't work with IE10?

Only my copy paste error of @hyperapp/http 🙃

zaceno commented 5 years ago

Thanks everyone who chimed in!

My conclusion is this:

I propose that Hyperapp adopts both the api and fetch based implementation of @okwolf 's hyperapp-fx ( src here ).

Why not a more advanced api? Elm's HTTP library is really involved!

Yes, but Elm is a different language, and must implement its own solutions for everything. Hyperapp really doesn't need to support every feature of every browser api, since any user with an "advanced" use case can simply make custom fx/subs.

It is important that we come out with something along with the release of Hyperapp 2, but it doesn't need to do everything. @okwolf's API seems good enough for most use case, and is easy to implement since it is close to the fetch api

We can always come out with a more advanced version of the effect (or an entire suite of HTTP-related effects & subs) later.

Why fetch? Why not XHR? IE10 has XHR support, and XHR can do stuff that fetch can't!

IE's XHR doesn't support json responseType, making it kind of useless anyway. fetch can easily be polyfilled in IE (which one imagines IE-supporters are accustomed to by this point).

As for the things XHR supports that fetch doesn't, see the answer above.


I would PR it, except that would involve making a package.json that I can't verify. Also the question of how to package & export the effect. I leave it up to @jorgebucaran to consider this suggestion 🙏

hlibco commented 4 years ago

@zaceno @okwolf One argument in favor of XHR is the native support for the timeout. Is this something to consider?

An alternative can be sending a signal using the AbortController() when using fetch(). The question is: Should this behavior be added in the effect or let users create their own custom http effects with timeout capabilities?

const controller = new AbortController();
const signal = controller.signal;

setTimeout(() => controller.abort(), 5000);

fetch(url, { signal }).then(response => {
  return response.text();
}).then(text => {
  console.log(text);
}).catch(err => {
  if (err.name === 'AbortError') {
    console.log('Fetch aborted');
  } else {
    console.error('Uh oh, an error!', err);
  }
});

Reference: https://developers.google.com/web/updates/2017/09/abortable-fetch

jorgebucaran commented 4 years ago

@hlibco Definitely. 👍

Here's an example I put together to show how we could use @hyperapp/http to track the progress of an http request. request returns an http effect, and track a subscription. The implementation would be based on fetch and AbortController for track.

import { app, text } from "hyperapp"
import { main } from "@hyperapp/html"
import { request, track } from "@hyperapp/http"

We also need a way to refer to multiple http requests.

const TrackerId = 1

This is just a function to create a specific http effect based on our desired parameters.

const getHyperapp = (action) =>
  request({
    tracker: TrackerId,
    method: "GET",
    url: "https://unpkg.com/hyperapp/hyperapp.js",
    headers: [],
    action,
  })

This is a helper function to set the status of the request, which can be: loading, success, or failure. Maybe it would be nicer if JavaScript had enums, but setStatus does the job.

const setStatus = ({ loading, success, failure } = {}) => ({
  loading,
  success,
  failure,
})

This action is only called when we have everything requested or if there was an error.

const GotSource = (state, { data, error }) => ({
  ...state,
  ...setStatus(error ? { failure: error } : { success: data }),
})

The track subscription will dispatch this action every time we receive stuff.

Hint: The same interface could be used for sending stuff, e.g., uploading (not covered in this example).

const Progress = (state, progress) => ({
  ...state,
  progress,
})

Here's the rest of the app.

app({
  init: [
    setStatus({
      loading: {
        tracker: TrackerId,
        progress: { received: 0, size: null },
      },
    }),
    getHyperapp(GotSource),
  ],
  view: (state) =>
    state.failure
      ? text("I was unable to load your book: " + state.failure.error)
      : state.loading
      ? text("Loading..." + state.loading.progress.received)
      : main({}, text(state.success.data)),

  subscriptions: (state) => [
    state.loading && track(state.loading.tracker, Progress),
  ],
  node: document.getElementById("main"),
})

Pattern matching would make this app way cooler to write, but not today.

mrozbarry commented 4 years ago

The thing that concerns me with this, is that fetch will resolve for any response from the remote. Including 404's et c. That means you'll get onBody callbacks even when there is no body, or the body is not of the bodyType you specified (in which case onErrorParsingBody will be dispatched).

Well, there are some APIs that may give json payloads on 4xx errors, including 404s, so you may still want to parse those, as they may give feedback to the application and user.

I think keeping things close to the metal would be good;

effects.Fetch({
  url: '...',
  options: {}, // whatever fetch accepts, including method, headers, etc.
  onResponse: someActionHere,
  onError: someActionHere,
})

The more API we build on top, the more we have to maintain, and the less control we give to the user. I could adventure with a little sugar on top to make things easier:

effects.Fetch({
  url: '...',
  options: {}, // whatever fetch accepts, including method, headers, etc.
  onOk: someActionHere,
  onError: someActionHere,
  okStatusCodes: [200, 201, 202, 203, 204, 205, 206, 207, 226],
  bodyType: 'text|json|blob|formData|arrayBuffer',
})

Behind the scenes, this would be something like:

const parsers = {
  text: response => response.text(),
  json: response => response.json(),
  blob: response => response.blob(),
  formData: response => response.formData(),
  arrayBuffer: response => response.arrayBuffer(),
};
const FetchFX = (dispatch, props) => {
  const parser = parsers[props.bodyType] || parsers.text;
  fetch(props.url, props.options)
    .then((response) => {
      if (!props.okStatusCodes.includes(response.statusCode)) {
        return parser(response)
          .then(body => dispatch(props.onError, { statusCode: response.statusCode, body })
      }
      return parser(response)
        .then(body => dispatch(props.onOk, { statusCode: response.statusCode, body });
    })
}

But I'd have a lot of caution adding too much on top of the default fetch implementation.

jorgebucaran commented 4 years ago

@mrozbarry The more API we build on top, the more we have to maintain, and the less control we give to the user.

We're on the same page. Userland belongs to userland.


@zaceno In my humble opinion, there needs to be an official fetch effect...

@hyperapp/http - Talk to servers, make http requests. When this issue was created I was still on the fence about providing scoped packages, but that's no longer the case! 💯

Or is that getting too complicated?

We should provide the bare minimum that works for Hyperapp. Elm's Http package will be a great source of inspiration. This example is a starting point.

hlibco commented 4 years ago

@jorgebucaran I'm curious to understand more about the API for the request().

In your example:

  request({
    tracker: TrackerId,
    method: "GET",
    url: "https://unpkg.com/hyperapp/hyperapp.js",
    headers: [],
    action,
  })

Will it be possible to return to the action all the information about the response including all the headers and not be limited to the status code or to the response body?

Ran4 commented 4 years ago

Here's an example I put together to show how we could use @hyperapp/http to track the progress of an http request. request returns an http effect, and track a subscription. The implementation would be based on fetch and AbortController for track.

Is there an updated version that works with the actual way request works?

In your example code your action function is:

const GotSource = (state, { data, error }) => ({
  ...state,
  ...setStatus(error ? { failure: error } : { success: data }),
})

but request's implementation today is:

export var request = fx((dispatch, props) => {
  var url = props.url
  var action = props.action
  var options = props.options || {}
  var expect = props.expect || "text"

  return fetch(url, options)
    .then(function(response) {
      if (!response.ok) {
        throw response
      }
      return response
    })
    .then(function(body) {
      return body[expect]()
    })
    .then(function(result) {
      dispatch(action, result)
    })
    .catch(function(error) {
      dispatch(action, error)
    })
})

That is, action will be called with (State, Data | Response), not (State, {Data | null, ErrorData | null}). The problem with this is that while it's simple enough to do a if (body instanceof Response), Response.body is a mutable promise that you can't read inside of the view function.

How are you supposed to read the json error message of a 4xx response in Hyperapp 2.0?

zaceno commented 3 years ago

@Ran4 I agree that the current http effect is not the ultimate for all use cases. I don’t think the response body is very commonly needed, for example (but sometimes: yes absolutely)

The good news is that making your own custom effects that do exactly what you want is easy.

But for Jorge making an official http effect that satisfies all the use cases will mean making an api that is pretty complicated and involves (how complicated? - look how elm does it and tremble)

From my perspective this api strikes a good balance. It is convenient to get started and once you need more than it offers, you just switch over to a custom effect that suits you. There’s never a reason to complicate your logic with an overworked generic api.

jorgebucaran commented 3 years ago

Eventually, we do want a more elaborate HTTP effect, very much like Elm's. We don't have one right now because designing one is challenging, but we'll get there. For now, using your own custom effect as a workaround is fine, though.

shish commented 3 years ago

How are you supposed to read the json error message of a 4xx response in Hyperapp 2.0?

I actually just hit this issue and it was pretty painful to figure out, but it now works reliably (I'm not sure about "elegantly", as there's like 4 layers of callbacks, and the code to handle "200 + json" vs "403 + json" are completely different... but it works)

(Using hyperapp-fx rather than with @hyperapp/http - the latter package.json exists, but appears to be missing an index.js? o.o;; )

My API returns data like

{
    "status": "ok",
    "code": 200,
    "messages": ["Successfully did the thing"],
    "data": {
        "cats": ["Alice", "Bob", "Cirilla"]
    }
}

As long as the server doesn't crash completely, then it'll return data in that format, whether it's 200 or 403 or 302 or whatever. If it does crash, then we just see <html><h1>Internal Error</h1></html>, and the code handles that case.

export function DisplayResponseMessage(dispatch, response) {
    function parseResponse(data) {
        // if we got 2XX or 3XX, show a temporary "ok" notification
        // which will disappear after 2 seconds
        if(data.code >= 200 && data.code < 400) {
            dispatch(state => [
                { ...state, notification: {text: data.messages[0], style: "ok"} },
                Delay({
                    wait: 2000,
                    action: (state) => ({...state, notification: null})
                })
            ]);
        }
        // if we got something else (4XX, 5XX), show an "error" notification
        // which doesn't go away until clicked
        else {
            dispatch(state => ({ ...state, notification: {text: data.messages[0], style: "error"} }));
        }
    }

    // When Http() is successful, "response" is the json from the server
    if(response.code) {
        parseResponse(response);
    }

    // When Http() fails, "response" is the raw blob, which we need
    // to decode for ourselves. First attempt to decode JSON (like
    // if the server sent us a 403). Fall back to "internal error" if
    // we can't decode it (like if the server crashed)
    else {
        response
        .json()
        .then(parseResponse)
        .catch(err => {
            console.log(err);
            dispatch(state => ({ ...state, notification: {text: "Internal Error", style: "error"} }));
        });
    }
}

Which gets used like:

            <button
                onclick={(state) => [
                    { ...state, loading: true },
                    Http({
                        url: "https://mysite.com/cats.json",
                        action: (state, response) => ([
                            {
                                ...state,
                                loading: false,
                                cats: response.data.list,
                            },
                            [DisplayResponseMessage, response],
                        ),
                        error: (state, response) => [
                            {
                                ...state,
                                loading: false,
                            },
                            [DisplayResponseMessage, response],
                        ],
                    }),
                ]}
                disabled={state.loading}
            >Get Cats</button>