refactor(hooks): refactor hooks to use composition (#10880) #12160

SimonSchick · 2020-04-23T23:27:55Z

#11727 # Pull Request check-list

Please make sure to review and check all of these items:

Does npm run test or npm run test-DIALECT pass with this change (including linting)?
Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
Have you added new tests to prevent regressions?
Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
Did you update the typescript typings accordingly (if applicable)?
Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Salvaged from previously suggested changes:

BREAKING CHANGE:

In order to streamline API:

All method style add hook functions have been removed in favor of a composition based approach.
Hook names have been removed, you can add and remove them by function reference instead which was supported before (this in line with browser and nodejs EventEmitter APIs)
this inside of hooks no longer refers to the the the hook subject, it should not be used.

This affects Model, Sequelize and Transaction.

Before: MyModel.beforeCreate(...)
After: MyModel.hooks.add('beforeCreate', ...)

Before: MyModel.addHook('beforeCreate', ...)
After: MyModel.hooks.add('beforeCreate', ...)

Before: MyModel.removeHook('beforeCreate', ...)
After: MyModel.hooks.remove('beforeCreate', ...)

Before: transaction.afterCommit(...)
After: transaction.hooks.add('afterCommit', ...)

Before:

MyModel.addHook('beforeCreate', 'named', fn);
MyModel.removeHook('beforeCreate', 'named');

After:

MyModel.hooks.add('beforeCreate', fn);
MyModel.hooks.remove('beforeCreate', fn);

Before: MyModel.addHook('beforeCreate', function() { this.someMethod(); });
After: MyModel.hooks.add('beforeCreate', () => { MyModel.someMethod(); });

eseliger

I like it and it even shaves off soo much code not needed to be maintained anymore!

types/test/models/User.ts

eseliger · 2020-04-23T23:47:14Z

lib/hooks.js

-    this.options.hooks = {};
-    _.map(hooks || {}, (hooksArray, hookName) => {
+  constructor(hooks, parent) {
+    this.hooks = {};


This seems like would be a good use-case for a Map 😁 But it's not really the scope of this PR so it's not blocking

Ideally maps would be used everywhere instead of objects, prototype pollution is becoming more and more of an issue.

Absolutely. Also, it has a performance benefit to it.

Maps are faster than objects?
I'll need some numbers 😛

https://jsperf.com/map-vs-object-sequelize/1 :)

@eseliger I was interested and gave it a test. Maps have a faster delete, but if just doing a set, then objects are faster.
https://jsperf.com/sequelize-map-vs-obj-2/1

Interesting :)
I also added reading the values in again, seems to still be faster (for those who are interested)
https://jsperf.com/sequelize-map-vs-obj-2/3

Interesting, in my yesterday benchmarks Maps were universally faster.

What operation were you looking at?

sushantdhiman · 2020-04-24T12:09:25Z

lib/hooks.js

- *
- * @param {string}   name
- * @param {Function} fn   A callback function that is called with instance, options
- * @name beforeValidate


We should preserve / update this documentation. It can be placed alongside respective hooksType entry

Can do, while I'm there, do you mind if we remove the params: <NUMBER> bit?
I just noticed this data is not used at all.

param indicate how many arguments are passed to hook, so you can deal with those arguments based on this data.

We can remove param if documentation is kept with hook definition, thus clearly stating what arguments are being passed

Will do, however expressing that 'clearly' is rather difficult with the limitation of jsdoc.
The typings will provide the most accurate API documentation here.

Just state what kind of object is being passed, jsdoc can handle that very well. Old docs from this file can be a starting point. Not everyone is using Typescript so we can't treat them as documentation

@sushantdhiman I did recover the documentation, probably not like you envisioned it though.

Feel free to give me an example because I really don't use jsdoc and have no idea how to model this accordingly.

/** * A hook that is run after validation * * @param {string} name * @param {Function} fn A callback function that is called with instance, options * @name afterValidate * @memberof Sequelize.Model */

This is what I was thinking, for each definition in hookTypes

docs/manual/other-topics/upgrade-to-v6.md

sushantdhiman

Moving away from mixin to this new approach is fine for me, Although I have to point out this is an unnecessary breaking change. We can perhaps deprecate old API first and introduce this new API alongside.

As mentioned in other comment, having this referring to current model / sequelize reference is useful and I don't think we should remove that feature

* refactor(hooks): remove method aliases for hooks BREAKING CHANGE: In order to streamline API: - All method style add hook functions have been removed in favor of a composition based approach. - Hook names have been removed, you can add and remove them by function reference instead which was supported before. - Another notable change that `this` inside of hooks no longer refers to the the the hook subject, it should not be used. This affects `Model`, `Sequelize` and `Transaction`. Before: `MyModel.beforeCreate(...)` After: `MyModel.hooks.add('beforeCreate', ...)` Before: `MyModel.addHook('beforeCreate', ...)` After: `MyModel.hooks.add('beforeCreate', ...)` Before: `MyModel.removeHook('beforeCreate', ...)` After: `MyModel.hooks.remove('beforeCreate', ...)` Before: `transaction.afterCommit(...)` After: `transaction.hooks.add('afterCommit', ...)` Before: ```js MyModel.addHook('beforeCreate', 'named', fn); MyModel.removeHook('beforeCreate', 'named'); ``` After: ```js MyModel.hooks.add('beforeCreate', fn); MyModel.hooks.remove('beforeCreate', fn); ``` Before: `MyModel.addHook('beforeCreate', function() { this.someMethod(); });` After: `MyModel.hooks.add('beforeCreate', () => { MyModel.someMethod(); });`

sushantdhiman · 2020-04-26T04:29:57Z

docs/manual/other-topics/upgrade-to-v6.md

+
+This affects `Model`, `Sequelize` and `Transaction`.
+
+#### Composition


Why are you calling these composition? :)

Lack of a better word since the hooks the hooks are now a composition rather than a mix in, probably should call this Examples instead.

sushantdhiman · 2020-04-26T04:35:43Z

lib/hooks.js

@@ -5,50 +5,96 @@ const { logger } = require('./utils/logger');
 const debug = logger.debugContext('hooks');

 const hookTypes = {
-  beforeValidate: { params: 2 },


We can also consider Model.hooks.beforeValidate(() => {]) in addition to Model.hooks.add('beforeValidate', () => {}), thus adding these methods directly in Hooks class. Syntax is bit shorter

Then we can document each method properly.

That'd kinda defeat the point of this tho, hooks don't know what they merely act as a promise event emitter.

Hooks that might be present in one context might not be in another.

sushantdhiman · 2020-04-26T04:43:00Z

lib/transaction.js

@@ -19,9 +21,9 @@ class Transaction {
   * @param {string} [options.deferrable] Sets the constraints to be deferred or immediately checked. PostgreSQL only
   */
  constructor(sequelize, options) {
+    this.hooks = new Hooks();


Wont this means, now you can do transaction.hook.add('beforeCreate') only to find it won't work, as only afterCommit hooks is invoked in this module.

We should find a way to throw with .add method.

As I mentioned in another comment, if we go with <object>.hooks.<hookName> approach this can be avoided

That is a good point, I tried to avoid this scenario with strict typings but not everyone uses that 😞 , however my comment #12160 (comment) still applies, adding methods to hooks for all possible hooks would be misleading and using inheritance would lead to a lot of bloaty code.

Example:

class ModelHooks extends Hooks { beforeCreate(a, b, c): Promise<void> { return this.run(a, b, c); // A lot of code } class SequelizeHooks extends ModelHooks { // ... }

I wanted to avoid making hooksspecific to their consumers but I guess this would work, I'd make therun` method private tho to avoid too much API surface and potential misuse.

SimonSchick · 2020-07-11T22:39:21Z

@eseliger any chance you'd be interested in picking this up, kinda bust atm.

eseliger · 2020-07-11T22:40:26Z

I thought it's been merged way ago haha. I can see if I find some time, but no promise

sdepold · 2021-10-24T18:29:24Z

Hey there :)

First of all: Thanks a bunch for your contribution to Sequelize! Much appreciated!
Second: Unfortunately we haven't had the chance to look into this ever since you created it. Sorry for that!

A couple of months ago, we have switched from master to main branch as our primary development branch and hence this PR is now outdated :(

If you still think this change is making sense, please consider recreating the PR against main. Thanks in advance and sorry for the additional work.

✌️

SimonSchick requested review from sushantdhiman, eseliger and papb April 23, 2020 23:27

eseliger approved these changes Apr 23, 2020

View reviewed changes

SimonSchick force-pushed the refactor/hooks branch 2 times, most recently from 04efb3e to ab45588 Compare April 24, 2020 00:53

SimonSchick changed the title ~~refactor(hooks): remove method aliases for hooks (#10880)~~ refactor(hooks): refactor hooks to use composition (#10880) Apr 24, 2020

sushantdhiman reviewed Apr 24, 2020

View reviewed changes

docs/manual/other-topics/upgrade-to-v6.md Outdated Show resolved Hide resolved

sushantdhiman suggested changes Apr 24, 2020

View reviewed changes

SimonSchick force-pushed the refactor/hooks branch from 4c952eb to 00057ac Compare April 25, 2020 18:36

sushantdhiman reviewed Apr 26, 2020

View reviewed changes

github-actions bot added the stale label Nov 2, 2021

WikiRik removed the stale label Nov 15, 2021

github-actions bot added the stale label Nov 30, 2021

SimonSchick closed this Jan 26, 2022

WikiRik deleted the refactor/hooks branch January 31, 2022 15:19

ephys mentioned this pull request Oct 15, 2022

feat: migrate hooks to TypeScript #15131

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(hooks): refactor hooks to use composition (#10880) #12160

refactor(hooks): refactor hooks to use composition (#10880) #12160

SimonSchick commented Apr 23, 2020

eseliger left a comment

eseliger Apr 23, 2020

SimonSchick Apr 24, 2020

eseliger Apr 24, 2020

SimonSchick Apr 24, 2020

eseliger Apr 24, 2020

SergeyCherman Apr 25, 2020

eseliger Apr 25, 2020

SimonSchick Apr 25, 2020

eseliger Apr 25, 2020

sushantdhiman Apr 24, 2020

SimonSchick Apr 24, 2020

sushantdhiman Apr 25, 2020 •

edited

sushantdhiman Apr 25, 2020

SimonSchick Apr 25, 2020

sushantdhiman Apr 25, 2020

SimonSchick Apr 25, 2020

sushantdhiman Apr 26, 2020 •

edited

sushantdhiman left a comment

sushantdhiman Apr 26, 2020

SimonSchick Apr 26, 2020

sushantdhiman Apr 26, 2020

SimonSchick Apr 26, 2020

sushantdhiman Apr 26, 2020

SimonSchick Apr 26, 2020

SimonSchick commented Jul 11, 2020

eseliger commented Jul 11, 2020

sdepold commented Oct 24, 2021


		This affects `Model`, `Sequelize` and `Transaction`.

		#### Composition

refactor(hooks): refactor hooks to use composition (#10880) #12160

refactor(hooks): refactor hooks to use composition (#10880) #12160

Conversation

SimonSchick commented Apr 23, 2020

Description of change

eseliger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sushantdhiman Apr 25, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sushantdhiman Apr 26, 2020 • edited

Choose a reason for hiding this comment

sushantdhiman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonSchick commented Jul 11, 2020

eseliger commented Jul 11, 2020

sdepold commented Oct 24, 2021

sushantdhiman Apr 25, 2020 •

edited

sushantdhiman Apr 26, 2020 •

edited