8000 Allow methods to return a null result by bertho-zero · Pull Request #865 · feathersjs/feathers · GitHub
[go: up one dir, main page]
Skip to content

Allow methods to return a null result#865

Merged
daffl merged 4 commits intofeathersjs:masterfrom
bertho-zero:null-result
May 4, 2018
Merged

Allow methods to return a null result#865
daffl merged 4 commits intofeathersjs:masterfrom
bertho-zero:null-result

Conversation

@bertho-zero
Copy link
Contributor
@bertho-zero bertho-zero commented May 3, 2018

I do not know if this modification will break the tests of other packages of the ecosystem (I do not think) but it seems normal to me to be able to return null value without it being an error.

In my case I need to return null when I get a softDeleted item instead of throw an error, the following code works with this PR:

// For return `null` value after a get on a soft deleted event I need to avoid error
error( context ) {
  if ( context.error instanceof errors.NotFound && context.error.message === 'Item not found.' ) {
    context.result = null;
    return context;
  }

  log.error(
    `Error in '${context.path}' service method '${context.method}'\n${context.error.stack}\n`,
    inspect( _.omit( context.error, [ 'hook.app', 'hook.service' ] ), { colors: true } )
  );
}

This can also be useful in a before hook for pass the service call and return null.

@bertho-zero bertho-zero changed the title Allow to methods to return a null result Allow methods to return a null result May 3, 2018
@bertho-zero
Copy link
Contributor Author
bertho-zero commented May 3, 2018

In my opinion result should be undefined, null can be a result.

@daffl
Copy link
Member
daffl commented May 3, 2018

Totally agree. Sorry, my edit broke the build. We should definitely have a test for this, too.

@daffl
Copy link
Member
daffl commented May 4, 2018 •
edited
Loading

Allright, added a tests and everything is passing now. Took me a bit to realize that this change only applies when setting context.result = null in an error hook, it already checked for undefined in the case of setting it in before.

@daffl daffl merged commit f83779f into feathersjs:master May 4, 2018
@daffl
Copy link
Member
daffl commented May 4, 2018

Released as 3.1.5, thank you!

@bertho-zero
Copy link
Contributor Author
bertho-zero commented May 4, 2018

Do not you think that a service call should be able to return null as a value? without going through the error hook.

This solution goes in the right direction but I am forced to create an error that is not one to change the result to null, which stops the chain of hooks in passing.

For me:

  • undefined = not defined
  • null = a possible result

@bertho-zero bertho-zero mentioned this pull request May 4, 2018
Closed
@daffl
Copy link
Member
daffl commented May 4, 2018

null return values for service should work just fine already.

@bertho-zero
Copy link
Contributor Author
bertho-zero commented May 4, 2018

You're right, after a more minimalist test it works.
(https://runkit.com/embed/aumy9wl9tzmz)

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

@bertho-zero @daffl
0