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 tests to cover hooks, and fix extraneous hook call #73

Merged
merged 3 commits into from
Dec 17, 2024

Conversation

botandrose
Copy link

There were no tests at all for the hooks, and hooks are about to get weird with the upcoming twoPass system, so I thought I'd add some to cover the status quo! However, this unearthed some bugs and questions.

So, three commits here:

  1. First one is covering all the hook behavior, as I expected it to work, based off the README. I had to mark three tests as skip, because...
  2. The beforeAttributeUpdated hook was erroneously getting called twice on additions and updates, with the second call claiming a remove mutation, which was definitely not happening. The change to the src in this PR is reflecting that fix.
  3. In the last commit I change the test from what I expected the behavior to be, to what it actually is. In short, afterNodeMorphed(oldNode, newContent) is being called with BOTH arguments essentially being the same. On one hand, this is surprising. I'd naively expect it to show me the before and after states, especially given the argument names. However, since the first argument is the node itself, and its after it's been morphed, it does make some sense for both arguments to be the same. So, is this a bug, or desired behavior? Maybe we just need to change the names of the arguments? Or just pass one (node) argument?

Lastly, two questions:

  1. I notice that all events have a before and after hook, except for the attribute update event, which only has a beforeAttributeUpdated event. Should we add a corresponding afterAttributeUpdated event? Pushing this to its logical conclusion, it would be easy to imagine expanding this to AttributeAdded AttributeUpdated and AttributeRemoved, as well. Has any downstream software ever wanted any of these?
  2. Looking at the arguments for beforeAttributeUpdated, I expected to see the value of the attribute in there, but I do not, just the name. Should we add it?

@1cg 1cg merged commit c1b548a into bigskysoftware:main Dec 17, 2024
1 check passed
@1cg
Copy link
Contributor

1cg commented Dec 17, 2024

  1. no strong feelings, just wanted a way to dynamically avoid updating attributes
  2. no strong feelings, I expected people to avoid updating attributes based on names, rather than values

@botandrose botandrose deleted the hook-tests branch December 18, 2024 18:27
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