Skip to content

Conversation

@raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented May 7, 2020

The NonVoid return type prohibits an interceptor function from having
void return type. This is important that an interceptor should either
return the value from downstream interceptors or a value from its own.

For example, the code will fail to compile.

 const myInterceptor: Interceptor = async (ctx, next) {
    // preprocessing
    // ...
 
    // subtle bug that the returned value from `next` is not returned
    // back to the upstream interceptors
    next();
 
   // postprocessing
    // ...
    // We must have `return ...` here
 }

Related to #5339

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

The `NonVoid` return type prohibits an interceptor function from having
void return type. This is important that an interceptor should either
return the value from downstream interceptors or a value from its own.
@raymondfeng raymondfeng force-pushed the force-interceptor-return-value branch from 38a2976 to 2aaf5c6 Compare May 7, 2020 03:57
Copy link
Contributor

@deepakrkris deepakrkris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, an explicit negative test case could be helpful

@raymondfeng
Copy link
Contributor Author

Unfortunately a negative case will fail to compile.

@raymondfeng raymondfeng merged commit 5663a76 into master May 7, 2020
@raymondfeng raymondfeng deleted the force-interceptor-return-value branch May 7, 2020 04:16
@bajtos
Copy link
Member

bajtos commented May 7, 2020

@raymondfeng

Unfortunately a negative case will fail to compile.

TypeScript 3.9 is adding ts-expect-error to support exactly this kind of tests, see https://devblogs.microsoft.com/typescript/announcing-typescript-3-9-beta/#ts-expect-error-comments

I am not sure if we are using 3.9 yet, there is an older (but less strict) alternative ts-ignore we can already use.


function logInterceptor(ctx: InvocationContext, next: Next) {}
function logInterceptor(ctx: InvocationContext, next: Next) {
return undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this redundant and a bit silly, because in JavaScript, functions return undefined when there is no return statement. I guess the extra verbosity is worth the stronger type safety your change provides.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants