Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PROPOSAL] Add component.handler middleware #1208

Merged
merged 16 commits into from
Mar 23, 2022
Merged

[PROPOSAL] Add component.handler middleware #1208

merged 16 commits into from
Mar 23, 2022

Conversation

jtfell
Copy link

@jtfell jtfell commented Jan 31, 2022

Proposed changes

We are building a custom analytics plugin to map components (rather than intents) to pages in Google Analytics. This diagram is specific to our use, but demonstrates what we want to achieve:

image

To do this, we would like to add a component.handler middleware so that plugins can monitor the use of components throughout a request. The way I've implemented it here seems to work. Our plugin does something like this:

  install(app: App): Promise<void> | void {
    app.middlewareCollection.use('after.component.handler', this.afterHandlerLogic);
  }

  private afterHandlerLogic = async (jovo: Jovo) => {
    const currentPage = jovo.$handleRequest?.activeComponentNode?.path?.join('/');

    const latestPage = jovo.$session.data.__LATEST_PAGE;
    if (currentPage !== latestPage && currentPage) {
      // TODO: Send pageview
    }
    jovo.$session.data.__LATEST_PAGE = currentPage;
  }

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

@jankoenig
Copy link
Member

Hi @jtfell, thanks a lot for this! I like the idea of tracking components as page views. Also, thank you for sharing your diagram, this is helpful for understanding the motivation behind this PR.

I was first a bit concerned because we'd like new middlewares to stay in our RIDR framework (right now, the component and handler execution happens in dialogue.logic: https://www.jovo.tech/docs/ridr-lifecycle#middlewares

However, since this is not an app middleware, I think this should work. Do you have more middlewares for component in mind? Would other component and handler specific middlewares be added like this?

  • component.start if a component is entered? Could be confused with the START handler
  • component.redirect or component.handler.redirect? Same for delegate, resolve, and send

Looking forward to hearing your thoughts on this! I'll add some documentation for this as well

@jankoenig jankoenig changed the base branch from v4/latest to v4/dev February 17, 2022 16:56
@jankoenig
Copy link
Member

I just restructured the component docs a bit (still not happy, but it should be a bit clearer now).

I also added the following:

  • ComponentTree section in components.md
  • Middlewares section in handlers.md

The documentation is very light there, suggestions appreciated 👍🏻

@jtfell
Copy link
Author

jtfell commented Feb 18, 2022

"However, since this is not an app middleware, I think this should work." - I'm not sure what you mean by this.

"Do you have more middlewares for component in mind? Would other component and handler specific middlewares be added like this?" - I haven't given it a great deal of thought but I can't think of any other use-cases that I would need this sort of thing for. Maybe some sort of monitoring solution (like APM traces) could benefit from it

Your ideas for further hooks seem reasonable, but I'm not sure exactly which ones would be most generally useful

I think the docs are looking good. I'll hopefully be able to contribute our analytics plugin at some point so that could be useful as an example to point to

@jankoenig
Copy link
Member

"However, since this is not an app middleware, I think this should work." - I'm not sure what you mean by this.

I meant those:

export const APP_MIDDLEWARES = [

@jtfell
Copy link
Author

jtfell commented Feb 21, 2022

I see, that makes sense. Thanks for clarifying. I've thought about this a bit more and I think that delegate, resolve, redirect and send could be useful for a few use cases:

@aswetlow
Copy link
Member

hey @jtfell . I really like the idea! However, I'm struggling with how to add this functionality into the framework in a consistent way. (middlewareCollection.run in all relevant functions).

We keep brainstorming 😄

@jtfell
Copy link
Author

jtfell commented Feb 22, 2022

Thanks for looking into it @aswetlow! What do you think of the latest commit I pushed as a starting point? Will that miss some edge cases? I'm still figuring out how all the pieces of the framework fit together so you'd know better than me

@jankoenig
Copy link
Member

Hi @jtfell, thank you for the update!

@aswetlow and I just talked about this:

  • We want to find a consistent way to add this to the framework architecture. We're currently thinking if middlewares like this could be called "Event Middlewares" (instead of the "App Middlewares" of the RIDR Lifecycle).
  • We're thinking if we could name the middlewares the same as the methods, for example event.executeHandler, event.$resolve, event.$redirect etc.

With this naming, we think it would make it a bit easier for us to add more hooks in the future. What do you think?

@jtfell
Copy link
Author

jtfell commented Feb 22, 2022

That all sounds great to me 👍

Feel free to take over this PR to implement it in this way :)

framework/src/Jovo.ts Outdated Show resolved Hide resolved
@jtfell
Copy link
Author

jtfell commented Feb 23, 2022

I've pushed a change that uses the following:

  • event.handler - (I think that executeHandler is a bit clunky, and not really part of the public API... Thoughts?)
  • event.$resolve
  • event.$redirect
  • event.$delegate
  • event.$send

@jankoenig
Copy link
Member

Hi @jtfell,

Thank you for your changes, I just added a "Middlewares" page to the docs.

After some consideration and discussions with @aswetlow, we decided to rename the handler middleware to event.ComponentTreeNode.executeHandler. Yes, it is a bit clunky and we're not 100% happy with it, but it leaves room for additional, granular middlewares in the future. We found event.handler easier to read, but a bit too generic.

This can be merged from my side, thank you!

@jankoenig jankoenig requested a review from aswetlow March 3, 2022 14:41
@aswetlow aswetlow merged commit 54e487b into jovotech:v4/dev Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants