aws-amplify / amplify-js

A declarative JavaScript library for application development using cloud services.
https://docs.amplify.aws/lib/q/platform/js
Apache License 2.0
9.42k stars 2.12k forks source link

(FEATURE FEEDBACK): Hub middleware #2669

Closed jkeys-ecg-nmsu closed 5 years ago

jkeys-ecg-nmsu commented 5 years ago

Is your feature request related to a problem? Please describe. In order to minimize the number of dependencies in our apps, we've been using Hub as our default pub-sub event broker. It works well with low latency for the most part, but I think it could be made more robust with middleware.

Describe the solution you'd like The ability to apply middleware to Hub that will allow for things like custom logging (e.g. on listener groups), and modifying listeners at runtime, grouping listeners that share logic in order to attach that logic to the listener group (DRY principle), garbage collection, etc.

For instance, consider a set of Show(.*) and Hide(.*) listeners (e.g. ShowAppSplashScreen and HideAppSplashScreen), each being created on a different Component that (as a constraint) has the same name as the capture group in the regex. Suppose also that each of these components, or their containers, provide show() and hide() methods. Then any listener that matches Show(.*) is going to execute its holder object's show() method when that event is sent.

Rather than duplicating that logic across every component by repetition in onHubCapsule, it would be cleaner to give Hub listener groups (perhaps by regex as in this example) and apply hooks that replace duplicated code.

In extreme cases, we have listeners that every component is listening to, like "UIReset", with every component that listens to it having a corresponding reset() method.

Something like the addition of these methods:

Hub.createGroup(listenerNames, groupName) - create a group aliased groupName from list listenerNames Hub.createGroupByRegex(regex, groupName), with a use like this:

Hub.createGroupByRegex(/Show(.*)/, "ShowListeners") - create a group referencable with ShowListeners that middleware can be attached to

Hub.attachMiddlewareToGroup(groupName, f) - apply function f to any listener l in group on Hub.dispatch(l, data), with a sample use case:

Hub.attachMiddlewareToGroup("ShowListeners", (capsule) => {
    let holder = capsule.holder //assume that every emit includes a reference to the holder object
    holder.show();
}

Hub.attachMiddlewareToGroup("HideListeners", (capsule) => {
    let holder = capsule.holder //assume that every emit includes a reference to the holder object
    holder.hide();
}

Then for the extreme use case, a similar method that lets you apply middleware to a singel listener:

Hub.attachMiddlewareToListener("UIReset", (capsule) => {
    let holder = capsule.holder //assume that every emit includes a reference to the holder object
    holder.reset();
}

This would allow us to put a lot of repetitive logic inside Hub and not repeat it across many components. If the holder reference for each object in the group could be provided by Hub as an additional parameter to the attachMiddleware* function(s), that would allow attaching middleware without modifying existing dispatch code. Like this:

Hub.attachMiddlewareToGroup("ShowListeners", (capsule, holder) => {
    holder.show();
    //process the capsule if necessary
}

Thoughts? I'm sure this idea can be drastically improved.

jkeys-ecg-nmsu commented 5 years ago

I haven't given a great deal of thought to the idea of letting web workers process events concurrently. There's a host of problems that goes along with that idea, e.g. how do you handle events which rely on being executed and completed in order if the callback is asynchronous or they're reading and writing to the same location.

jkeys-ecg-nmsu commented 5 years ago

@jordanranz in looking for more robust evening solutions, I stumbled on RFP and BaconJS. Do you think Bacon could be used to merge Hub events into streams such that logic can be combined into EventStream objects?

undefobj commented 5 years ago

Hello @jkeys-ecg-nmsu. I've looked into this a bit. We had already been looking into some possible improvements for Hub so your suggestions are timely. There have been some patterns we were considering including changing Hub into a full blown Observable, but right now we don't want to introduce any breaking changes and are unsure of the long term benefits here. However I think we can make some changes to still support your goals.

I have created a new version of the Hub module with some sample usage and posted it in the following repo: https://github.com/undefobj/hub_reboot

You'll find a readme there with sample usage, and you can also download it and run some tests. Please let me know your feedback if this is in the direction you were looking for to improve your usage of the module, any changes in functionality you might require, or any future ideas for improvement. Thanks.

undefobj commented 5 years ago

I haven't given a great deal of thought to the idea of letting web workers process events concurrently. There's a host of problems that goes along with that idea, e.g. how do you handle events which rely on being executed and completed in order if the callback is asynchronous or they're reading and writing to the same location.

Do you have any use cases that require data to be processed in-order? The current thought in this new implementation is not to enforce any callback ordering or allow mutating the payload by middleware you introduce. There would be some potential implications of this which worry me:

  1. It could have security implications, such as impacting Auth flows
  2. It would require a stricture function prototype of the middleware, such as requiring a next() function
jkeys-ecg-nmsu commented 5 years ago

Do you have any use cases that require data to be processed in-order? The current thought in this new implementation is not to enforce any callback ordering or allow mutating the payload by middleware you introduce. There would be some potential implications of this which worry me:

I have no such use cases in mind, but can see how others might, eg using a channel to serialize network calls or shard data. That's probably a rare edge case though.

Just FYI, making "channel" a String type will not break our codebase, but we currently use payload to send unstructured objects (as in, no "name" and "data" fields) in a number of cases, but refactoring that should be simple enough. I think it's safe to say that the payload.data field will be an object at least some of the time. I wonder if any problem with that could be solved by serializing it with JSON.stringify on dispatch and deserializing it with JSON.parse as a default middleware on every listen?

undefobj commented 5 years ago

I have no such use cases in mind, but can see how others might, eg using a channel to serialize network calls or shard data. That's probably a rare edge case though.

Ok I think for now I'm going to consider this a possible future enhancement unless we hear more feedback to the contrary this next week or so.

Just FYI, making "channel" a String type will not break our codebase, but we currently use payload to send unstructured objects (as in, no "name" and "data" fields) in a number of cases, but refactoring that should be simple enough. I think it's safe to say that the payload.data field will be an object at least some of the time. I wonder if any problem with that could be solved by serializing it with JSON.stringify on dispatch and deserializing it with JSON.parse as a default middleware on every listen?

Do you have an example of what your objects look like? E.g. how many levels deep the keys are nested, etc.

jkeys-ecg-nmsu commented 5 years ago

From the looks if it very flat, usually unnested objects.

jkeys-ecg-nmsu commented 5 years ago

@undefobj since this isn't getting much feedback, maybe you could just create a new RFC thread and unpin this one?

undefobj commented 5 years ago

@jkeys-ecg-nmsu I think that's ok, not everyone is using Hub in anger yet and we're looking to slowly evolve it. Is the current functionality in the sample repo good for you? Or would you like us to make changes based on the payload structure above, or any other functionality that you would need for your functionality.

Another thing we have been thinking about is potentially using Hub with some state management libraries like Redux (or in place of them). Interested in your thoughts there.

jkeys-ecg-nmsu commented 5 years ago

I think that's ok, not everyone is using Hub in anger yet and we're looking to slowly evolve it. Is the current functionality in the sample repo good for you? Or would you like us to make changes based on the payload structure above, or any other functionality that you would need for your functionality.

The payload structure you've proposed is fine, and honestly I should have enforced a similar payload object interface in the first place.

I do have concern with replacing the supplied callback with the holder's onHubCapsule method if it exists; I think it would be more stable to console.warn that a legacy onHubCapsule implementation exists and will probably be deprecated in the future, and then let the holder invoke both the supplied callback and the legacy method.

Another thing we have been thinking about is potentially using Hub with some state management libraries like Redux (or in place of them). Interested in your thoughts there.

My goal has been to use Hub and the API category to eliminate the need for enterprise-smelling state management libraries. I would love to see Hub become a full-blown alternative for client-side state management. Your proposal of making Hub an Observable is certainly interesting.

jkeys-ecg-nmsu commented 5 years ago

@undefobj @jordanranz To me, the ideal (event-driven) application is one where every piece of state is reactive to all relevant event streams. In BaconJS parlance, every piece of state should be a Property.

If literally every piece of state of your application is a Property reacting to EventStreams, doesn't that make your application very easy to reason about (assuming your Rx library is correct) while making it even more declarative? And since AppSync/managed GraphQL services offer subscriptions, the possibility of making an entirely event-driven application with minimal imperative sections seems greater than ever.

What are your all's thoughts on reactive programming as the primary means of managing dataflow for event-driven applications?

jkeys-ecg-nmsu commented 5 years ago

@undefobj, I love everything you've done since you first posted your reboot. Warning onHubCapsule is deprecated, dedicating a data key for pattern matching, etc. I think it looks fantastic! Thank you very much.

jkeys-ecg-nmsu commented 5 years ago

@undefobj Just to clarify, in terms of "middleware", the new idea is that you would listen for multiple callbacks on the same RegExp group?

Also, if my callback needs a reference to the calling object (e.g. to call a method that all calling objects will have as a constraint, like show() and hide()), should I assume that I need to pass in a reference to the calling object as a property of the payload object? Like:

const payload = {
   data: /Show(.)*/,
   caller: this,
   ...otherData
}
Hub.dispatch("ShowMyComponent", payload, "MyComponent");
...
const myRegexGroup = /Show(.*)/
Hub.listen(myRegexGroup, (capsule) => {
  capsule.caller.show()
})

I'm just trying to clarify what payload structure needs to internally be enforced in the future. (I may be advocating for a company-wide move to a typed front-end language soon; working without typechecking puts my brain into overdrive.)

undefobj commented 5 years ago

@undefobj Just to clarify, in terms of "middleware", the new idea is that you would listen for multiple callbacks on the same RegExp group?

Also, if my callback needs a reference to the calling object (e.g. to call a method that all calling objects will have as a constraint, like show() and hide()), should I assume that I need to pass in a reference to the calling object as a property of the payload object? Like:

const payload = {
   data: /Show(.)*/,
   caller: this,
   ...otherData
}
Hub.dispatch("ShowMyComponent", payload, "MyComponent");
...
const myRegexGroup = /Show(.*)/
Hub.listen(myRegexGroup, (capsule) => {
  capsule.caller.show()
})

I'm just trying to clarify what payload structure needs to internally be enforced in the future. (I may be advocating for a company-wide move to a typed front-end language soon; working without typechecking puts my brain into overdrive.)

@jkeys-ecg-nmsu we're pretty close to merging this and are going to do some reviews and testing since it touches a fair amount of category surface area. In answer to your questions could you take a look at the PR for docs that I just submitted in preparation for this release: https://github.com/aws-amplify/docs/pull/544 I'll probably add more but I think this will answer many of your asks.

Also if you would like to test I've published the code to our beta tag in npm: yarn add aws-amplify@beta aws-amplify-react@beta

jkeys-ecg-nmsu commented 5 years ago

onHubCapsule seems to be broken, not just deprecated.

Anytime I try to call this.setState from onHubCapsule, I get this error:

[ERROR] 07:56.325 Hub TypeError: this.setState is not a function

This isn't a major problem, and represents a good time to refactor away from onHubCapsule, but just FYI the beta release is semi-breaking.

undefobj commented 5 years ago

@jkeys-ecg-nmsu can you give us a code sample?

jkeys-ecg-nmsu commented 5 years ago
    constructor(props) {
        super(props);

        this.state = this.getInitialState();

        this.handleDone = this.handleDone.bind(this);
        this.handleMoreAboutCaregivers = this.handleMoreAboutCaregivers.bind(this);
        this.handleMoreAboutAddisonClick = this.handleMoreAboutAddisonClick.bind(this);
        Hub.listen("ShowAppAddisonQuestionsSidebar", this, "AppAddisonQuestionsSidebar");
        Hub.listen("HideAppAddisonQuestionsSidebar", this, "AppAddisonQuestionsSidebar");

        Hub.listen("ShowAppAddisonQuestionsSidebar", this, "AppAddisonQuestionsSidebar");
        Hub.listen("ShowAppAddisonQuestionsSidebar", this, "AppAddisonQuestionsSidebar");

        Hub.listen("ShowMoreAboutAddison", this, "AppAddisonQuestionsSidebar");
        Hub.listen("ShowMoreAboutCaregivers", this, "AppAddisonQuestionsSidebar");

        Hub.listen("DoneCommand", this, "AppAddisonQuestionsSidebar");
        Hub.listen("UIReset", this, "AppAddisonQuestionsSidebar");
    }

    onHubCapsule(capsule) {
        const { channel, payload } = capsule;

        if(channel === "UIReset") {
            this.setState(this.getInitialState())
        }

        else if(channel === "ShowAppAddisonQuestionsSidebar") {
            this.setState({on: true});
        }
        else if (channel === "HideAppAddisonQuestionsSidebar") {
            this.setState({on: false})
        } else if(channel === "ShowMoreAboutCaregivers"){
            console.log("Showing More About Caregivers");
            this.setState({aboutCaregiversOn: true, aboutAddisonOn: false})
        } else if(channel === "ShowMoreAboutAddison"){
            console.log("Showing More About Caregivers");

            this.setState({aboutCaregiversOn: false, aboutAddisonOn: true})
        } else if(channel === "DoneCommand"){
            Hub.dispatch("UIReset");
            Hub.dispatch("ShowInitSidebar");
        }
    }

...

    render() {
        return this.state.on ? (
            <div>
                <div id="appOptionsSidebarContainer" style={this.containerStyle}>
                <button style={this.buttonStyle} className="btnTemplate" onClick={this.handleMoreAboutAddisonClick}> More about yourself</button>
                <button style={this.buttonStyle} className="btnTemplate"  onClick={this.handleMoreAboutCaregivers}> More about Caregivers </button>
                <button style={this.buttonStyle} className="btnDone"  onClick={this.handleDone}> Done </button>
                </div>
            </div>
        ) : (null);
    }

AppUserInterface.jsx:

//AppUserInterface.jsx

import React from 'react';
import { Hub } from 'aws-amplify';
import { Bacon } from 'baconjs'; // eslint-disable-line no-unused-vars
import { AppSplashScreen, AppButtons, DevButtons, AppGameContainer } from './containers'; 
import { AppInitialSidebar, AppHealthSidebar, AppAddisonQuestionsSidebar, AppAboutPrompt } from './components';

export default class AppUserInterface extends React.Component {
    getInitialState() {
        return {
            appButtonsOn: true,
            devMode: true,
            initialSidebarOn: true,
            healthSidebarOn: false,
            aboutMenuOn: false,
        }
    }

    constructor(props) {
        super(props)

        this.state = this.getInitialState();

        this.onAppButtonsChange = this.onAppButtonsChange.bind(this);
        this.onDevButtonsChange = this.onDevButtonsChange.bind(this);
        this.handleHealthClick = this.handleHealthClick.bind(this);
        this.handleAboutClick = this.handleAboutClick.bind(this);
        this.handleHelpClick = this.handleHelpClick.bind(this);
        this.handleWebsiteClick = this.handleWebsiteClick.bind(this);
        // this.setState = this.setState.bind(this);
        this.getInitialState = this.getInitialState.bind(this);

        // const SHOW_REGEX = /Show(.*)/;

        Hub.listen("ShowAppInitialSidebar", (data) => {
            console.log("in AppUserInterface, ShowAppInitialSidebar.data: ", data);
            const { payload } = data;

            this.setState({initialSidebarOn: true});
        })

        Hub.listen("HideAppInitialSidebar", (data) => {
            console.log("in AppUserInterface, ShowAppInitialSidebar.data: ", data);
            const { payload } = data;

            this.setState({initialSidebarOn: false});
        })

        Hub.listen("UIReset", (data) => this.setState(this.getInitialState()));
    }

    handleHealthClick(e) {
        e.preventDefault();
        // window.amplifySceneLoader.dispatchPlayAsthmaMedReminder();
        // Hub.dispatch("loadAsthmaMedReminder");
        this.setState({initialMenuOn: false, healthMenuOn: true})
    }

    handleAboutClick(e) {
        e.preventDefault();
        // window.amplifySceneLoader.dispatchPlayAsthmaMedReminder();
        // Hub.dispatch("loadAsthmaMedReminder");
        this.setState({initialMenuOn: false})
        Hub.dispatch("ShowAppAddisonQuestionsSidebar");

    }

    handleHelpClick(e) {
        e.preventDefault();
        window.amplifySceneLoader.dispatchPlayHelpScene();
        //window.amplifySceneLoader.emit("loadHelpScreen");
        window.eventManager.emit("loadHelpScreen");
        Hub.dispatch("loadHelpScreen");
    }

    handleWebsiteClick(e) {
        e.preventDefault();
        this.state.eventManager.emit("post_to_lex", "birthday");
    }
    //pull in any data that sub-containers will need
    async componentDidMount() {

    }

    onAppButtonsChange(event) {

    }

    onDevButtonsChange(event) {

    }

    render() {
        return (
        <React.Fragment>
            <AppSplashScreen />

            <AppInitialSidebar on={this.state.initialSidebarOn} handleTopClick={this.handleHealthClick} handleMiddleClick={this.handleAboutClick} /> 
            <AppHealthSidebar on={this.state.healthSidebarOn} />
            <AppAboutPrompt on={this.state.aboutMenuOn} />
            <AppAddisonQuestionsSidebar />

            <AppButtons onChange={this.onAppButtonsChange} on={this.state.appButtonsOn}/>
            <DevButtons onChange={this.onDevButtonsChange} on={this.state.devMode} /> 

            <AppGameContainer />
        </React.Fragment>
        );
    }
}

The component that kicks it all off, AppInitialSidebar.jsx:

//AppInitialSideBar.jsx

import React from 'react';

export default class AppInitialSidebar extends React.Component {
    containerStyle = {
...
    };

    promptStyle = {
...
    };

    buttonStyle = {
...
    }

    render() {
        return this.props.on ? (
            <div>
                <div id="appOptionsSidebarContainer" style={this.containerStyle}>
                    <button style={this.buttonStyle} className="btnTemplate" onClick={this.props.handleTopClick}> Medication Management </button>
                    <button style={this.buttonStyle} className="btnTemplate"  onClick={this.props.handleMiddleClick}> Tell me about yourself </button>
                </div>
            </div>
        ) : (null);
    }
}
[ERROR] 13:38.343 Hub TypeError: this.setState is not a function
    at Object.onHubCapsule [as callback] (AppAddisonQuestionsSidebar.jsx:44)
    at eval (Hub.js:153)
    at Array.forEach (<anonymous>)
    at HubClass._toListeners (Hub.js:149)
    at HubClass.dispatch (Hub.js:101)
    at AppUserInterface.handleAboutClick (AppUserInterface.jsx:65)
    at HTMLUnknownElement.callCallback (react-dom.development.js:147)
    at Object.invokeGuardedCallbackDev (react-dom.development.js:196)
    at invokeGuardedCallback (react-dom.development.js:250)
    at invokeGuardedCallbackAndCatchFirstError (react-dom.development.js:265)
    at executeDispatch (react-dom.development.js:571)
    at executeDispatchesInOrder (react-dom.development.js:596)
    at executeDispatchesAndRelease (react-dom.development.js:695)
    at executeDispatchesAndReleaseTopLevel (react-dom.development.js:704)
    at forEachAccumulated (react-dom.development.js:676)
    at runEventsInBatch (react-dom.development.js:844)
    at runExtractedEventsInBatch (react-dom.development.js:852)
    at handleTopLevel (react-dom.development.js:5025)
    at batchedUpdates$1 (react-dom.development.js:20915)
    at batchedUpdates (react-dom.development.js:2246)
    at dispatchEvent (react-dom.development.js:5105)
    at interactiveUpdates$1 (react-dom.development.js:20977)
    at interactiveUpdates (react-dom.development.js:2267)
    at dispatchInteractiveEvent (react-dom.development.js:5081)
undefobj commented 5 years ago

@jkeys-ecg-nmsu the reason you're getting this error is because you're using setState inside of onHubCapsule which is bound to this from your React component and React doesn't autobind. To use setState like that (even in the current version of Hub today) you would need to do this.onHubCapsule = this.onHubCapsule.bind(this) inside your constructor before Hub.listen() is called. If you didn't use setState in your onHubCapsule you wouldn't see this error as this wouldn't be trying to reference a function in that component. Hope this makes sense.

jkeys-ecg-nmsu commented 5 years ago

@undefobj got it. I thought I had made sure onHubCapsule was bound across all components (I've found this to be the primary cause of Hub v1.0's error delivering... warning).

undefobj commented 5 years ago

@jkeys-ecg-nmsu this is now live: https://aws-amplify.github.io/docs/js/hub

jkeys-ecg-nmsu commented 5 years ago

Thanks, we've already switched back from beta branch. We (or at least I) really appreciate all your hard work!

github-actions[bot] commented 3 years ago

This issue has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.