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

Add custom endpoints to extensions #1613

Merged
merged 2 commits into from
Aug 27, 2023
Merged

Conversation

tschicke
Copy link
Contributor

Thanks for merging #1600. This is a small WIP addon to that, that adds the ability for extensions to register their own endpoints that can be called at '/beancount/extension/<ext_name>/', so that the template html or the new js module can communicate with the extension backend, which I think opens up the possibility for much more advanced extensions. This is a pretty bare-bones version of the change that doesn't handle arguments particularly well, and might have issues with locking (I'm not sure how locking around requests works), but it has been working well for me in a few personal extensions that I've written in the past few weeks.

I'm not sure if this type of thing is something you'd want to support, so I wanted to see if this was interesting at all before spending too much time cleaning it up.

@tschicke tschicke marked this pull request as draft May 23, 2023 23:34
@yagebu
Copy link
Member

yagebu commented May 24, 2023

I'm not sure if this type of thing is something you'd want to support

Sure, let's add this if you have a use case for it.

src/fava/application.py Outdated Show resolved Hide resolved
src/fava/application.py Outdated Show resolved Hide resolved
src/fava/core/extensions.py Outdated Show resolved Hide resolved
@yagebu
Copy link
Member

yagebu commented May 24, 2023

This fixes #1530

@tschicke tschicke force-pushed the extension_endpoints branch from da0ed0d to 46a9959 Compare May 29, 2023 00:45
@tschicke tschicke marked this pull request as ready for review May 29, 2023 00:49
@tschicke
Copy link
Contributor Author

This fixes #1530

Oh thanks, I didn't notice there was already an issue for this.

@tschicke
Copy link
Contributor Author

tschicke commented May 29, 2023

I made the extension_endpoint decorator the main way for extensions to register endpoints. This can be used either without arguments (endpoint name = function name, methods = ["GET"]):

@extension_endpoint
    def update_status(self):
        ...

Or with arguments (endpoint name, methods):

@extension_endpoint("update", methods=["POST"])
    def update_endpoint(self):
        ...

I also created an ExtensionApi class in the frontend. This gets set on each extension module, and had get, put, post, and delete functions to call endpoints within the extension.

The last thing might be overkill, but it looks through all elements with class name "extension-handler" and goes through each "data-handler-" attribute, converting the value to a handler for that event that has access to the extension module. This normally wouldn't be necessary, and you could just set an id on the required elements and init the event handlers in onExtensionPageLoad, but this makes it easier when the html template creates dynamic elements, like in a loop, where the handler for each element needs the loop variable, e.g. from one of my extensions:

{% for key, value in extension.status()|dictsort %}
    <tr>
      <td>{{ key }}</td>
      <td><pre>{{ value }}</pre></td>
      <td><button class="extension-handler" data-handler-click="this.api.post('disable', {name:'{{ key }}'})">Disable</button></td>
      <td><button class="extension-handler" data-handler-click="this.api.post('enable', {name:'{{ key }}'})">Enable</button></td>
    </tr>
{% endfor %}

Without something like the "data-handler" thing, it's more difficult to do something like {name:'{{ key }}'} and you need to do something like store the key somewhere and refer back to it later. I think it cleans some stuff up, but I understand if it's a little too hacky (like the need for // eslint-disable-next-line @typescript-eslint/no-implied-eval)

I still need up update the documentation and add an example of the functionality to an included extension, but I wanted to get your opinion on this direction before doing that.

@tschicke tschicke force-pushed the extension_endpoints branch from 46a9959 to 1a46e10 Compare June 3, 2023 00:28
Copy link
Member

@yagebu yagebu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last thing might be overkill, but it looks through all elements with class name "extension-handler" and goes through each "data-handler-" attribute, converting the value to a handler for that event that has access to the extension module.

I'm not a fan of that - I generally dislike inline event handlers. I also think the frontend code should be mostly agnostic of how the extension does frontend logic (unless it's reusing some existing Fava logic so I could imagine having some more direct access to the Svelte components or custom stuff like the router) - it could do so using a full-fledged frontend-rendering framework like React or Svelte or for ergonomics closer to HTML with inline handlers something like htmx or Stimulus.

frontend/src/extensions.ts Outdated Show resolved Hide resolved
frontend/src/extensions.ts Outdated Show resolved Hide resolved
@tschicke tschicke force-pushed the extension_endpoints branch from 36cd918 to 4e0d739 Compare August 5, 2023 22:06
@tschicke
Copy link
Contributor Author

tschicke commented Aug 5, 2023

I'm not a fan of that - I generally dislike inline event handlers. I also think the frontend code should be mostly agnostic of how the extension does frontend logic (unless it's reusing some existing Fava logic so I could imagine having some more direct access to the Svelte components or custom stuff like the router) - it could do so using a full-fledged frontend-rendering framework like React or Svelte or for ergonomics closer to HTML with inline handlers something like htmx or Stimulus.

No problem, I figured it was probably too much of a hack. I can achieve most of that functionality in an extension that uses onPageLoad, since like I mentioned some sort of ability to specify things inline makes working with loops in extension html much easier for now.
I think making something like React or Svelte accessible to the extension frontend would be the ideal way to solve this, but Svelte would require shipping the Svelte compiler with fava, and invoking it at runtime, which seemed like too much, and adding a runtime framework like React or one of the others seemed confusing since the core frontend already uses Svelte.

@andreasgerstmayr
Copy link
Contributor

I think making something like React or Svelte accessible to the extension frontend would be the ideal way to solve this, but Svelte would require shipping the Svelte compiler with fava, and invoking it at runtime, which seemed like too much, and adding a runtime framework like React or one of the others seemed confusing since the core frontend already uses Svelte.

Tangentially related: I once did a PoC of loading React in an extension: https://github.com/andreasgerstmayr/fava-dashboards/blob/react/frontend/src/app.tsx. Registering custom routes would be awesome, as currently it's writing JSON in a <script> tag and loading that to bootstrap the React app.

@tschicke
Copy link
Contributor Author

tschicke commented Aug 8, 2023

I can achieve most of that functionality in an extension that uses onPageLoad
After playing around with this, the one extra thing I need in order to do this in an extension rather than fava is an ability to get other extensions from within an extension. I added the getExt function to the context so that any extension can call that to query for an instance of another extension if they need to.

@tschicke tschicke force-pushed the extension_endpoints branch from 6ccb190 to d49c495 Compare August 8, 2023 00:51
@tschicke tschicke force-pushed the extension_endpoints branch from d49c495 to 725511e Compare August 20, 2023 08:23
@yagebu
Copy link
Member

yagebu commented Aug 27, 2023

I think making something like React or Svelte accessible to the extension frontend would be the ideal way to solve this, but Svelte would require shipping the Svelte compiler with fava, and invoking it at runtime, which seemed like too much, and adding a runtime framework like React or one of the others seemed confusing since the core frontend already uses Svelte.

There's no need for invoking the Svelte compiler at runtime - if you want to build Svelte components, your extension should have a build process and if you want to use Fava's components, we can expose those to extensions. It's really up to the extension to pick a frontend framework and use it.

The extension_endpoint decorator allows extensions to register
endpoints.

It also adds the ExtensionContext and class which is passed to all
extension module functions. The context currently only contains an
ExtensionApi, which allows the extension to call it's endpoints more
easily.
@yagebu yagebu force-pushed the extension_endpoints branch from a5e67dd to 81d39aa Compare August 27, 2023 08:48
@yagebu yagebu force-pushed the extension_endpoints branch from 81d39aa to ddda62f Compare August 27, 2023 08:57
@yagebu yagebu merged commit 6b8782f into beancount:main Aug 27, 2023
16 checks passed
@tschicke
Copy link
Contributor Author

tschicke commented Sep 1, 2023

I think making something like React or Svelte accessible to the extension frontend would be the ideal way to solve this, but Svelte would require shipping the Svelte compiler with fava, and invoking it at runtime, which seemed like too much, and adding a runtime framework like React or one of the others seemed confusing since the core frontend already uses Svelte.

There's no need for invoking the Svelte compiler at runtime - if you want to build Svelte components, your extension should have a build process and if you want to use Fava's components, we can expose those to extensions. It's really up to the extension to pick a frontend framework and use it.

Thanks, yeah I realized that was the intended way to do that after seeing how https://github.com/redstreet/fava_investor works. I have 5+ local extensions that I'm using/testing with so adding svelte/react to a bunch of different extensions would add a bunch of boilerplate, so I was trying to come up with a way that fava could do some of that work for me. But I agree that the flexibility of letting extensions use whatever frontend they want is better.

@tschicke tschicke deleted the extension_endpoints branch September 1, 2023 09:11
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