8000 fix: Clean up hooks code by vonagam · Pull Request #1407 · feathersjs/feathers · GitHub
[go: up one dir, main page]
Skip to content

fix: Clean up hooks code#1407

Merged
daffl merged 1 commit intofeathersjs:masterfrom
vonagam:fix-clean-hooks
Jun 30, 2019
Merged

fix: Clean up hooks code#1407
daffl merged 1 commit intofeathersjs:masterfrom
vonagam:fix-clean-hooks

Conversation

@vonagam
Copy link
Member
@vonagam vonagam commented Jun 18, 2019

Fixes two errors:

  1. finally hooks can possibly run twice for same method call if one of them throws. Also a finally hook if throws activates error hooks, this is in conflict with this comment and also Promise.finally and try/catch/finally convention. Because currently it is like this (1, 2):
try {
  run([...before])();
  method();
  run([...after, ...finally])();
} catch {
  run([...error, ...finally])();
}
  1. An error hook can get a context which is not up to date, if any before hook returns new context object and a service method throws, because errors (1, 2) thrown in a method invocation do no have hook property set (which should contain a last valid hook context), that's why error hooks end up using an initial hook object.

Added fixes and tests for such cases. Also small clean up changes for readability and performance.

A question for future: What are use cases for allowing returning new context object from hook functions where modifying context in place is not as convenient?

@vonagam vonagam force-pushed the fix-clean-hooks branch 3 times, most recently from 35389f8 to d417ff7 Compare June 18, 2019 14:42
@vonagam
Copy link
Member Author
vonagam commented Jun 18, 2019

What about adding finally to the list of hook types?

@daffl daffl merged commit f25c88b into feathersjs:master Jun 30, 2019
@daffl
Copy link
Member
daffl commented Jun 30, 2019 •
edited
Loading

This makes a lot of sense, thank you! That the finally hooks are not documented is somewhat intentional because they were really only intended to be used internally (to send events). For the next major version I'd prefer to go in the direction the Koa style hook system outline in #932 which would allow a finally like this:

app.service('users').hooks({
  create: [
    async (context, next) => {
      const finally = async () => {
        // Finally code here
      }

      try {
        await next();
        await finally();  
      } catch (error) {
        await finally();
        throw error;
      }
    }
  ]
}

A convenience wrapper may also make this shorter.

@vonagam
Copy link
Member Author
vonagam commented Jun 30, 2019

Cool about koa style, prefer it too.

By the way your example snippet has the same problem as this pull request solves for current system: if your finally() function throws it will run twice. Here is how it should be written:

Instead of:

try {
  await next();
  await finally();  
} catch (error) {
  await finally();
  throw error;
}

It should be:

try {
  await next();
} finally {
  await finally();
}

@daffl
Copy link
Member
daffl commented Jun 30, 2019

Ah, totally. That's totally it.

@vonagam vonagam deleted the fix-clean-hooks branch July 2, 2019 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vonagam @daffl
0