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

prepend keeps index of existing handler, or jumps ahead of aos handlers (_eval, and _default) #357

Open
dtfiedler opened this issue Sep 26, 2024 · 3 comments

Comments

@dtfiedler
Copy link

dtfiedler commented Sep 26, 2024

When a handler previously exists via Handler.add, Handler.append etc, and then is modified to Handler.prepend it's current index is preserved, making it only prepend the handlers that follow in the ordered Handler list. This seems counterintuitive when iterating on handlers and electing to move a handler up in the Handlers.list. Additionally, when a prepended handlers is introduced, it is place in first index of the Handlers list which could jump over any default handlers provided by AOS. This seems particularly risky in the case of _eval where one could brick a process by prepending a handler before _eval with bad code, and then having no way to replace the code out of it.

A few thoughts:

  • aos provided handlers by default, should remaining beginning of the Handlers list (e.g. _default, _eval, etc. are always the first handlers)
  • Handlers.prepend should move a handler to after the private handlers (_eval and _default) unless explicitly stated to keep it's current position or to be placed higher than the private ones

Related code: https://github.com/permaweb/aos/blob/main/process/handlers.lua#L107-L114

@twilson63
Copy link
Collaborator

Great points, maybe _default and _eval should be removed from handlers list and implictly applied.

handle
_eval

...

for each
handlers.list
end

...

_default

end


re-updating handlers pattern and handle function if already exists is by design, whether you call prepend, add, etc. Not sure if it makes sense to change this, it is built this way so developers can re-eval their scripts, seems to work for many.

@dtfiedler
Copy link
Author

dtfiedler commented Sep 26, 2024

I like the idea of them being implicit, seems sensible - with that change, would you consider not allowing Handlers with those names to avoid overriding them?

I also understand not wanting to update the handler patterns, but would suggest at the very least additional documentation around the behavior if they already exist. this behavior surprised us when we realized the handler we moved to prepend did not change positions in the list and only executed before later handlers.

I can make an initial PR to the docs - mind pointing me to the correct spot?

@twilson63
Copy link
Collaborator

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

No branches or pull requests

2 participants