Feature unnamed hook used as a reference

[harm-less]

Hey,

Thanks for the plugin, I've added an additional feature I hope you like.

The reason I made this PR is because I wanted to be able to create a hook reference that wasn't based on a name. This allowed me to use typed hooks inTypeScript!

An example:

Console output:

As you can see, the User interface is being passed along to the before and the actual hook method. So exporting the saveHook reference allowed me to use the hook in the different file but with strong typing.

I also improved the typings file by adding Promises where necessary, otherwise the before hook in my example started to complain about the async keyword.

I'm not super happy about having to make a seperate unnamed method for this, but I was forced to because the name properly can be a string or a string array. And there was no way to alter the register method in such a way that its parameters could vary between 2 and 4 parameters and still function consistently in all scenarios as the options. And then again, the before, after, error and wrap methods still needed to know what hook reference to talk to without providing a name, so I'm not sure there even is a way.

Maybe you have some improvements you can think off, but I already spend too much time trying to figure out a Typescript related bug. Turned out I was using a using an old version of Typescript that caused it... So sorry for the additional "debug" commits 😅

[gr2m]

Wow that’s quite a big pull request.

I think it would be better to start out with an issue or maybe a pull request to the README to discuss what you want to do before putting in so much work :)

I couldn’t yet wrap my head around it. You need the .unnamed() method because of Typescript? I wish I could better understand why it’s not possible with the current API, but I’m only starting to learn Typescript.

I feel like your change is so big, you could make your own library which drops the named hooks altogether?

[harm-less]

Hehe, yeah, might be a bit big, but I'm reusing a lot of existing code to make it possible I believe. Though, I don't think it should be it's own library, in my opinion it's just an additional feature. Don't you think there's too many packages out there already? 😅

I also didn't update the readme or anything as I wanted your opinion first.

But I'll try to explain why I think it's a good feature and why it's not optimal with the current api.

Why I think this feature is a good addition
The best way I can think of is directly explaining it from the index.d.ts. All of the in- and output of the methods you currently have in the index.d.ts typings file have any. In other words, you simply cannot "pass along" the data type of a hook automatically when using it. For example:

The options parameter in the hook method has a datatype of any as that's stated in your definitions. So there's no limit what I can do with it, everything is allowed as you can see from the hook method's body.

And the same is true for the before or after hooks:

Now, with the unnamed hook reference, we will get autocompletion and validation:

As you can see, the options object is always of type FooInterface while calling the hook or applying any of the methods like before or after. If something is not in correspondence with the original interface type, it's going to complain:

As you can see, the huge advantage of this is that you can have your code implementing the hooks and your code that handles all the before and after login seperate while keeping it 100% typed. Therefore TypeScript will tell you once a change in one area is an issue in a different area. Awesome!

And of course you can use this feature too when using plain JS, so it isn't just a feature for TypeScript.

An alternative with the current API
The only way you could solve it without my additions I think (after changing the typings a bit so I can pass along the FooInterface):

You pretty much get the same behavior type wise with less code, but it also means you still have to pass along the name all the time and within a single HookInstance you cannot change the type. In other words, you cannot use a different data type for a save action and let's say a delete action. If you want to do that, you need to create another Hook instance.

Conclusion
The Hook instance you need to pass along in your application anyway, so I rather have single hook I pass along instead of the entire hook collection. The hook name is already embedded in the name of reference if you name your hooks right, so I feel I rather leave it out than having to type it everytime I use the before or after methods.

I hope this explanation makes more sense now and you see the point I'm trying to make ;)

[gr2m]

Don't you think there's too many packages out there already?

Never :) It depends on what you optimize for. I use this in Octokit, the JavaScript tooling for GitHub’s APIs. The library is used in both browsers and servers. Particularly for browsers, the bundle size is relevant, so I’d always prefer smaller speciallized libraries over libraries that have a lot of features I don’t use

I hope this explanation makes more sense now and you see the point I'm trying to make ;)

It’s fantastic, thank you so much! I kinda wish we came up with this in the first place, I feel everything I need to do with the current API I could do with .unnamed() as well, and the typings would sure be a great help to prevent mistakes.

E.g. current in Octokit, I have this

octokit.before('request', beforeRequest)

I could as well do this in future

octokit.request.before(beforeRequest)

So okay, let’s do it, you convinced me :)

I wonder if instead we could do something like this

import Hook from 'before-after-hook/unnamed'
const saveHook = new Hook<user>()

I think that way the current bundle would remain the same because it wouldn’t bundle in the new unnamed functionality.

Now, we need to document the new API. And I’m not sure if unnamed is the right name 🤔Naming is hard.

[harm-less]

Valid point regarding the bundle size, but I guess it will always remain a double-edged sword :)

I'm glad I convinced you, whoe! Now all we got to do is come up with the best implementation and semantics 👍

Suggestion one
Rename unnamed() to ref(). Simple and shorter, but effective I suppose. Its unnamed nature is implied but that will be documented of course.

Suggestion two
An alternative could be to make two constructors:

1. Replace Hook with the unnamed version: new Hook<User>()
2. Rename the current Hook with: new Hooks() or new HookCollection(). Because I also feel the current Hook constructor is more of a collection of hooks rather than a single hook. This was bit confusing when I first started using the library.
   This of course will be a breaking change.

Suggestion three
Your suggestion about make a sub-package. This does require you to expose some key components in your current package, but it doesn't make any breaking changes.
However, we are talking about an additional 20 - 25 lines of code that might potentially even be optimized, I feel like it would be a lot of trouble to go through just for that.

I lean towards suggestion 1 for simplicity and no breaking changes, option 2 as mixture of ease and completeness API wise, but definitely to three for perfection 😄

[ci-reporter]

The build is failing

✨ Good work on this PR so far! ✨ Unfortunately, the Travis CI build is failing as of b544c5f. Here's the output:

npm test

> before-after-hook@0.0.0-development pretest /home/travis/build/gr2m/before-after-hook
> standard

standard: Use JavaScript Standard Style (https://standardjs.com)
standard: Run `standard --fix` to automatically fix some problems.
  /home/travis/build/gr2m/before-after-hook/index.js:35:30: Extra semicolon.

I'm sure you can fix it! If you need help, don't hesitate to ask a maintainer of the project!

---

This comment was automagically generated by ci-reporter. If you see a problem, open an issue here.

[gr2m]

Rename the current Hook with: new Hooks() or new HookCollection(). Because I also feel the current Hook constructor is more of a collection of hooks rather than a single hook. This was bit confusing when I first started using the library.

You are totally right!

I think we can use that for naming as well, the current API is a collection, the new API is singular.

I think eventually this API would be nice, but require a breaking change

// singular
const userHook = new Hook<User>()
userHook.before(validate)
userHook(options, save)

// collection
const hooks = new Hook.Collection()
hooks.before('save', validate)
hooks('save', options, save)

Would that make sense? 🤔

In order to get there, we could introduce a temporary API new Hook.Singular() and log a deprecation message when using new Hook() mentioning to use new Hook.Collection() instead, as the behavior for new Hook() will change to what new Hook.Singular() does with the next breaking version. Then we can deprecate new Hook.Singular() with the next breaking version.

[harm-less]

I like it! I think that fixes most of the inconsistencies we've discussed so I agree with all of your suggestions.
Let me apply the changes necessary and finalize the details for this pr. Hopefully I can get it done by today :)

[harm-less]

All right, I've made the changes. It might look a little messy, but without the code for the deprecation warnings, it should look nice.
I think this API works quite well. Let me know if you like the code and if you do, I'll get started on finishing up the type definitions and the readme 🚀

[harm-less]

All right, I think I covered every angle 🎉

I've thought about copying the collection API in the readme and remove all the name argument descriptions, but then I thought that would cause a lot of documentation duplication which I rather don't do. So I hope you like what I did here by explaining that you can simply read the collection API and ignore the name argument everywhere. Do you think people will understand this sufficiently?

As for the unit tests. I think the unit tests are covering it very minimally, but most of the code is covered because of the collection tests so I don't think it a huge problem. I think in your next major release you can rewrite the unit tests to better fit what it should test. Also, I'm not very familiar with this particular unit tests syntax, so, therefore, I'm a bit reluctant to go crazy there 😉

Hope you like what you read!

[gr2m]

Just a few minor comments.

The workaround for the memory leak is so straight forward, great job 👍

[harm-less]

Glad you liked my global variable fix, though I must admit I like smoothies too! 😄

I've used the version from yesterday (today didn't change a lot of logic) in my code and it works very nicely. I guess one last good look at the code and we should be ready to go, right?

[harm-less]

Is there anything left to discuss or improve?

[gr2m]

Nope. I’ll test it with @octokit/rest next and if everything looks good, I’ll ship it

[gr2m]

[gr2m]

🎉 This PR is included in version 1.4.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[harm-less]

That was a quick test, haha. But thanks for merging, it was a fun process we went through ;)