jorgebucaran / hyperapp

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

V2 Middleware API #753

Closed jorgebucaran closed 5 years ago

jorgebucaran commented 6 years ago

Summary

I've made up my mind about this issue. I'm going with the dev-only middleware option. That would be the πŸŽ‰ in @okwolf's two-choice poll. This should tie in with https://github.com/jorgebucaran/hyperapp/issues/417. I'll close there and summarize everything here.

Thank you everyone, for your feedback! ❀️

Technically, this isn't middleware, I've been calling it so incorrectly. Hyperapp will have no middleware feature as we know it from other projects such as Redux or Express. In the past we've used plugins (pre-V1) to integrate to ReduxDevTools and later V1 wiredActions for the same purpose. V2 will provide its own DevTools instead.

We want a top-class development experience when working with Hyperapp. Tools like okwolf/logger, mrozbarry/debug and hyperstart/devtools have helped us along the way and will be the inspiration for what's coming next.

Starting with V2 we'll be able to compile our apps for production or development. Development mode will have useful warnings, logging, session import/export and time-traveling built right into your app!

This will work by setting an environment variable, e.g., NODE_ENV=production or NODE_ENV=development. What will be default?

For CDN users, hyperapp.js will be the development build and a new hyperapp.min.js will be available.

okwolf commented 6 years ago

I've held off on making the first comment here since I felt I sucked up most of the oxygen in the last issue about middleware.

I think the right way to kick this off is to decide the scope of when middleware should be used in the first place.

The @jorgebucaran perspective on this has already been voiced:

The kind of middleware that I want to see is the least invasive kind, the one that allows you to build a logger or a time travel debugger, but not modify how dispatch works in order to, among other things, add Promise support to actions, etc.

Perhaps we should kick this off with a small poll (inspired by the action parameter comment). Please react to this comment accordingly:

(I intentionally didn't use πŸ‘ or πŸ‘Ž to avoid bias from perceiving one as better or worse.)

If the dev-only option wins, perhaps we should consider only exposing middleware functionality in a potential development build of Hyperapp.

mrozbarry commented 6 years ago

I think there is value in doing more complicated and potentially production side middleware. I agree that promise based actions aren't necessary, but I could see things like error reporting and analytics being good candidates for production middleware.

jorgebucaran commented 6 years ago

Thanks, @okwolf @mrozbarry.

I think V2 middleware should exist to intercept actions for inspection purposes, not to modify how dispatch works.

e.g. (just a suggestion, I'm not particularly sold on this approach)

app(obj => { console.log(obj) })({ init, view, subscriptions, container }) 
screen shot 2018-09-06 at 22 17 23

@mrozbarry I could see things like error reporting and analytics being good candidates for production middleware.

If your effect or subscription can fail, then it should accept an error action (in addition to at least one action to be dispatched with the EffectResult or SubscriptionEvcent) where you can handle the error, e.g., update the state to display that an error occurred, send a crash report to a server and so on.

Similarly, effects may be used to add Google Analytics to an app.

zaceno commented 6 years ago

@jorgebucaran

should exist to intercept actions for inspection purposes, not to modify how dispatch works.

... then perhaps "middleware" isn't the best name to use for this. Most places I've seen, "middleware" is expressly for the purpose of injecting processing steps in a data-flow.

Maybe something like "tap" or "probe" or "inspection" or something is more appropriate?

zaceno commented 6 years ago

Perhaps simply like:

app({
  init: ...,
  view: ...,
  container: ...,
  tap: (action, newState, prevState) => {
   ....
  }
})
SkaterDad commented 6 years ago

@zaceno I like the "tap" idea. It clearly defines that this function can't affect the real data flow.

I think dev-mode logging and analytics are the only use case I would need this for, so I don't have much to add.

jorgebucaran commented 6 years ago

@SkaterDad Why not use effects #750 to add analytics to a page?

SkaterDad commented 6 years ago

@jorgebucaran Why not use effects #750 to add analytics to a page?

It's been difficult to come up with ideas for middleware use cases, so I just threw it out there.

Effects are probably the right way to do it, so you don't flood the server with unnecessary events.

okwolf commented 6 years ago

So here are a few of the production use cases where I would have thought to reach for middleware as a solution summarized:

jorgebucaran commented 6 years ago

Nice list @okwolf, thanks for writing this. Here are my comments for each use case you described.

Persisting state

Why would it be awkward to use subscriptions for this? It's only reasonable. I claim it is the opposite. Any approach not based in a subscription model should be declared awkward. :trollface:

To write to storage you need a subscription to notify an action of every state change. The action will create the effect that writes to storage. Or create an effect that throttles the action/effect that writes to storage.

To read from storage you'll create the effect within the init action when running the app, from a user-generated event, subscription, etc., or both. There will be only one action that creates the effect that reads from storage. You'll either use it as your init action or as the fx/subscription action or both.

Merge state "slices" (modules)

I'm against this use of middleware. Why? It allows you to change how Hyperapp works making it too easy for different codebases to become incompatible. Accelerated unwarranted app speciation.

Analytics

Another use case for effects as you've pointed out. I suggest we create an issue to discuss only tracking and analytics in V2.

For async errors, some sort of FX helper would probably be in order. This would be somewhat annoying having to wrap all FX that can fail instead of implementing in one place.

Why? Do you also wrap fetch to use a shared onReject/onError handler? πŸ€”

Undo / Redo

This is what the NODE_ENV=development built-in debugger will exist for, among other things like logging and creating universal paradoxes.

Interoperate with events from Observables / callbags

Like subscriptions, observables and callbags are a layer of abstractionβ€”they wrap a lower-level API.

Let's say you just released FusionWarp a new micro-service. You are offering a traditional callback-style and a promise-based API to your users. As it turns out someone already built an observable and a callbag adapter for FusionWarp. Good for them!

Now someone wants to try FusionWarp with Hyperapp. Using FusionWarp observable / callbags adapter with Hyperapp doesn't make sense. They are competing technologies.

Now, there is no fx/subscription adapter for Hyperapp either. Solution: You'll need to make one (and share it with the world). Your subscription will be based in the FusionWarp API, not the observable or callbag!

Dispatching Promises

Very funny.

okwolf commented 6 years ago

Thanks for the thorough reply @jorgebucaran. Here are my responses:

Persisting state

@jorgebucaran Why would it be awkward to use subscriptions for this? It's only reasonable. I claim it is the opposite. Any approach not based in a subscription model should be declared awkward. :trollface:

Awkward in that this would not be a subscription like most of the others users are accustomed to. The typical use case for the state parameter is to vary the return from subscriptions like so:

subscriptions: state => state.isTracking && [Mouse.moves({ action: MouseMoved })]

but for this case you would also need to pass the state itself to the sub.

subscriptions: state => [
  Persist(state),
  state.isTracking && Mouse.moves({ action: MouseMoved })
]

Make sure you don't destructure on that state either, or you won't have the entire state available to pass:

subscriptions: ({ isTracking }) => [
  Persist(state), // OOOPs!!
  isTracking && Mouse.moves({ action: MouseMoved })
]

Of course you could improve on this with a higher-order subscriptions function if you wanted:

const withPersist = subscriptions => state => [
  Persist(state),
  ...subscriptions(state)
];

// meanwhile, in user code...
subscriptions: withPersist(
  ({ isTracking }) => isTracking && [Mouse.moves({ action: MouseMoved })
]

Persist in my examples is an bit of an odd beast of a subscription too. Unlike most subs, it would only call dispatch once, at the time of creation. And each sub returned from Persist would need to be different enough so that the effect would be called each time the state updates. In all other cases, this would be considered a bad practice for a subscription. The implementation might look something like:

const localStorageEffect = props => {
  localStorage.setItem("state", JSON.stringify(props.state));
};

const PersistLocalStorage = state => [
  state,
  { state, effect: localStorageEffect }
];

const Persist = state => ({
  state,
  // ordinarily this should be defined separately as a best practice
  // since this is what causes each object returned to be considered a unique sub
  // and thus re-run this effect
  effect(props, dispatch) {
    dispatch(Time.debounce({ action: PersistLocalStorage, wait: 1000 }));
  }
})

Merge state "slices" (modules)

@jorgebucaran I'm against this use of middleware. Why? It allows you to change how Hyperapp works making it too easy for different codebases to become incompatible. Accelerated unwarranted app speciation.

I think this is a compelling argument, considering we are moving into a relatively opinioned space in the framework ecosystem. Enforcing some level of consistency is not a bad idea here. 🍻

Analytics

@jorgebucaran Another use case for effects as you've pointed out. I suggest we create an issue to discuss only tracking and analytics in V2.

I completely agree. I'm not sure if we'll ever have a "blessed" analytics library implemented by one of the contributors, but it would be nice to give some guidance for anyone who wants to write such a thing.

@jorgebucaran Why? Do you also wrap fetch to use a shared onReject/onError handler? πŸ€”

My point here was that if we had some standard prop on FX (say onError) for what action to call if an error/failure occurs, middleware would be able to detect this and automatically attach an error reporting effect to this action before/after the user supplied one.

Undo / Redo

@jorgebucaran This is what the NODE_ENV=development built-in debugger will exist for, among other things like logging and creating universal paradoxes.

When I said undo/redo, I meant as a user functionality, not for debugging.

Ctrl + Z and Ctrl + Y in many apps.

Interoperate with events from Observables / callbags

πŸ‘ to the @jorgebucaran response on this one. Nothing to add or disagree with.

Dispatching Promises

@jorgebucaran Very funny.

Trolls gonna troll. :trollface:

okwolf commented 5 years ago

@jorgebucaran is this something you expect will be available in 2.0, or in a later 2.x release?

To the question in the revised summary: I think it would make sense for the default in the Node.js environment to be production for performance reasons, even though React uses development by default.

jorgebucaran commented 5 years ago

@okwolf It will be an incremental feature. Logging the action name and data will be available at first, and so on.

mrozbarry commented 5 years ago

@jorgebucaran we should probably re-open this, and then we can close PR #803 since the code in there isn't relevant.

For context, while debug code could live in the hyperapp repo, I think it became obvious in my PR that a debugger probably shouldn't live in core.

I'd be happy with something like:

app({
  devOnlyOnDispatch: (action, args) => {
  },
});

That would give me more than enough to integrate a debugger, and doesn't expose anything in core that could mean changing the behaviour of hyperapp.

okwolf commented 5 years ago

I'm curious if we should advertise this as a dev-only feature, or merely the officially-supported solution for subscribing to dispatched actions/FX. I believe there are legitimate use cases for this in production, as I said the last time we discussed middleware.

We should provide more than just the arguments to the dispatch call, like the state value and what was returned by the action, since these would allow for easier detailed tracing of what's happening for a debugger.

This wouldn't allow for a few of the more nifty features of Redux/Elm devtools like time travel debugging, dispatching dynamic user-provided actions, or restoring saved state snapshots. These features could be enabled via an accompanying subscription, however.

selfup commented 5 years ago

I really do still love the simplicity of an exposed dispatch. Especially for interop. I get subs, but an exposed dispatch is very attractive. Thanks for bringing it up!

jorgebucaran commented 5 years ago

V2's latest beta now has a middleware hook which can be enabled by passing a second argument to the app function. This argument is itself a function that transforms Hyperapp's internal dispatch function.

You can use it as follows:

import { h, app } from "hyperapp"

const enhance = oldDispatch => newDispatch

app(props, enhance)

or through a higher order function like this:

export const withEnhancements = app => props => {
  const myEnhancer = oldDispatch = newDispatch
  return app(props, myEnhancer)
}
import { h, app } from "hyperapp"
import { withEnhancements } from "./withEnhancements"

withEnhancements(app)(props)

I hope we use this feature to create loggers, time-traveling debuggers, devtool integrations, and the best of @okwolf ideas from https://github.com/jorgebucaran/hyperapp/issues/753#issuecomment-419678324. Let's use this to improve Hyperapp's debugging experience, not change how Hyperapp works! πŸ˜‰

See also: https://github.com/jorgebucaran/hyperapp/pull/822. Thank you, @mrozbarry! πŸŽ‰

sergey-shpak commented 5 years ago

@jorgebucaran correct me if I'm wrong, it's not possible to overwrite internal dispatch function with a custom one (you won't get correct fn scope), middleware hook is only a wrapper around the dispatch

mrozbarry commented 5 years ago

@sergey-shpak you should be able to create a completely different dispatch function, but keep in mind that if you do, it would be hard for the community to give help and support for unpredictable behaviour.

sergey-shpak commented 5 years ago

@mrozbarry it is not clear how to create a new dispatch function with a scope of internal one, I mean, if I want to call 'setState' from new dispatch function - I can't, because I don't have reference to 'setState' (it is in a different scope). or am I missed something?

mrozbarry commented 5 years ago

I think Carl Sagan put it best:

Basically, you want to make a new dispatch, you have to make a new setState, and in making a new setState, you have to make a new subscriptions.

Dispatch is quite literally the brain of the whole application, so creating a new dispatch is essentially saying you want to create a new brain for Hyperapp.

What are you trying to accomplish by creating a completely different dispatch?

jorgebucaran commented 5 years ago

@sergey-shpak If you create a new dispatch mechanism, you can set the state by using the original dispatch function we pass into the middleware/wrapper function. In this way, you have indirect access to setState.

Please use this feature to inspect Hyperapp's data-flow, not change how it works.

sergey-shpak commented 5 years ago

@mrozbarry , @jorgebucaran I don't want to change dispatch functionality) I'm clarifying because I want to be sure that no one else can )

pekeler commented 4 years ago

A few things have changed since this discussion ended. Does the decision to use middleware only for dev still stand? If so, I'm wondering how I could implement undo for my users. Writing an action enhancer that records the next state into a state.history, and then applying this enhancer to all my actions seems a bit tedious.

If it's OK now to use middleware for production, I wish dispatch() would be refactored into two functions. A public dispatch() that calls the private _dispatch() and then setState(). The middleware enhances the private _dispatch(), i.e. it can change state. And if the current state became an additional parameter to _dispatch(), an undo middleware could be quite straightforward to implement.

const undoMiddleware = (dispatch) => (action, props, state) => ({
  present: dispatch(action, props, state.present),
  history: [...(state.history || []), state.present]
})

i.e. with an implementation along these lines:

var _dispatch = (props.middleware || identity)(function(action, props, state) {
  return ...
})
var dispatch = function(action, props) {
  return setState(_dispatch(action, props, state))
}
jorgebucaran commented 4 years ago

@pekeler Alas, I think it's fine to use middleware for production. This is an old discussion, though, so the info is outdated. The current middleware signature is:

const middleware = dispatch => /* newDispatch */

app(props, middleware)