paked / messenger

Package messenger is used for making bots for use with Facebook messenger
MIT License
274 stars 74 forks source link

WIP: NLP, CTX, more control over handle and custom http (per request needs) #41

Closed dfischer closed 5 years ago

dfischer commented 5 years ago

Using Google App Engine I had to make modifications to this lib to load it per request. Additionally I wanted to access the Facebook Messenger Wit.ai Integration NLP attributes.

Changes:

  1. Made handle public so creating a middleware layer is easier.
  2. Moved Handler logic/registry to a package global and it's own file instead of being built on Messenger struct. Without this change then handler initialization would be per request which doesn't make much sense.
  3. Removed non-standard API items from messenger ProfileFields (this should probably be a config object, maybe a TODO for this PR).

Todo:


Example of how I am using it:

func main() {
    // Setup server
    s := newServer()

    // Setup messenger handlers
    messenger.Handlers.HandleMessage(s.messageHandler)

    // Setup router
    http.HandleFunc("/webhook/messenger", s.addEnv(s.messengerHandler))

    // Starts server to receive requests
    appengine.Main()
}

func (s *server) messengerHandler(w http.ResponseWriter, r *http.Request) {
    ctx := r.Context()
    http := ctxHTTP(ctx)

    m := messenger.New(messenger.Options{
        // AppEngine requires urlfetch. Set it in 3rd party lib
        Client:      http,
        Verify:      s.config.messenger.Verify,
        AppSecret:   s.config.messenger.AppSecret,
        VerifyToken: s.config.messenger.VerifyToken,
        Token:       s.config.messenger.Token,
    })

    // Pass incoming messages off to the messenger lib handler
    m.Handle(w, r)
}
paked commented 5 years ago

I've been thinking about this PR and came to the following conclusion.

I haven't used Go in a while, and have fallen behind the curve in terms of what is "good API design" in that community. Subsequently I don't think I'm the right person to be making these sorts of decisions in regards to this package.

So, I'm going to do this: When you think this PR is ready to be merged, I will merge it. If people have a problem with the changes, it will be up to them to raise an issue and suggest alternatives.

If you are going to continue to work on this repository I will make you a collaborator, since I've never really been a fan of "abandoned" projects resulting in fork after fork after fork after fork (all of which become just as "abandoned" as the original source). That way I can not be a bottleneck for incoming changes, and the repository can stay in the same place. If it makes sense to add more collaborators, I'd be happy to do so.

dgellow commented 5 years ago

Hi @dfischer, @paked. I have a suggestion for this pull request. In my opinion it is trying to change too many things at the same time, could we split it and instead have multiple small PRs each focused on one set of change? Some of them can be approved and merged quickly (NLP, profile fields), and other changes will surely require some discussion.

paked commented 5 years ago

@dgellow: This is a good idea.

@dfischer: would you mind?

dfischer commented 5 years ago

@paked thank you - I’m also in appreciation of the work you put in here and don’t want to step on toes. I’m carefully considering the changes as well. I will sit on it throughout the weak. No need to rush.

@dgellow I agree. I will look into this during the week.

Thanks for the feedback. I’ll iterate and patch.

dfischer commented 5 years ago

Will reopen with a new PR soon.