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:masterf 8000 rom
    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