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

channels: custom hooks #4165

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from
Draft

channels: custom hooks #4165

wants to merge 4 commits into from

Conversation

arthyn
Copy link
Member

@arthyn arthyn commented Nov 12, 2024

PR Checklist

  • Includes changes to desk files
  • Describes how you tested the PR locally (test ship vs livenet)
  • If a new feature, includes automated tests
  • Comments added anywhere logic may be confusing without context

Copy link
Member

@Fang- Fang- left a comment

Choose a reason for hiding this comment

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

Partial comments. We should continue at some point (from $result and onward).

Comment on lines +41 to +42
+$ action
$% [%add name=@t src=@t cron=(unit @dr)]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
+$ action
$% [%add name=@t src=@t cron=(unit @dr)]
+$ action
$: =nest:c
$% [%add name=@t src=@t cron=(unit @dr)]
...
== ==

Comment on lines +25 to +31
:: $hooks: collection of hooks, the order they should be run in, and
:: any delayed hooks that need to be run
++ hooks
$: hooks=(map id hook)
order=(rev (list id))
delayed=(map id delayed-hook)
==
Copy link
Member

Choose a reason for hiding this comment

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

As written this is per-channel (many-to-one), but could be worth doing many-to-many, so you don't have the experience of needing to "copy" hooks into other channels, but rather just enabling them for the other channels.

Something like this:

Suggested change
:: $hooks: collection of hooks, the order they should be run in, and
:: any delayed hooks that need to be run
++ hooks
$: hooks=(map id hook)
order=(rev (list id))
delayed=(map id delayed-hook)
==
:: $hooks: collection of hooks, the order they should be run in, and
:: any delayed hooks that need to be run
++ hooks
$: hooks=(map id hook)
order=(map nest:c (list id))
delayed=(map id [for=nest:c delayed-hook])
==

And then probably pull .enabled out of $hook and just make that implicit in hooks being present in the .order for any given $nest.

This also gives you the benefit of being able to do "global" state in a hook. If the hook cares about only operating in the local context, it just keeps a (map nest:c hook-state) instead. (So tbc, state.hook would remain just a $vase.)

$: =id
name=@t
enabled=?
src=(rev src=(unit @t))
Copy link
Member

Choose a reason for hiding this comment

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

We probably want a "written for" version tag alongside the source code, so that we can do backcompat for all the trivial cases (and possibly for everything).

Comment on lines +102 to +107
+$ event
$% [%on-post on-post]
[%on-reply on-reply]
[%cron ~]
[%delay delayed-hook]
==
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
+$ event
$% [%on-post on-post]
[%on-reply on-reply]
[%cron ~]
[%delay delayed-hook]
==
+$ event
$% [%on-post on-post]
[%on-reply on-reply]
[%cron ~]
[%wake delayed-hook]
==

Maybe %wake here, and then %wait for the $effect below.

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.

2 participants