Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use isObervable from rxjs instead of our own implementation. Drop few deprecated APIs from 3rd-party libs. And other minor refactorings #8851

Merged
merged 6 commits into from
Dec 27, 2021

Conversation

micalevisk
Copy link
Member

@micalevisk micalevisk commented Dec 24, 2021

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Issue Number: closes #8595

As you can see here: https://cs.github.com/nestjs/nest?&q=isObservable there are few versions of isObservable on this codebase. Which are implemented as follow:

function isObservable(input): input is Observable<any> {
  return input && isFunction(input.subscribe)
}

I'm under assumption that all of them are supposed to return true when the input is an Observable object created by rxjs lib.

What is the new behavior?

Using isObservable type guard from rxjs instead (introduced in rxjs@6)

image

which is implemented as follows here:

function isObservable<T>(obj: any): obj is Observable<T> {
  return !!obj && (obj instanceof Observable || (isFunction(obj.lift) && isFunction(obj.subscribe)))
}

thus, I believe this should fix the linked Issue.

I've fixed the return type of Server#transformToObservable as well:

before

image

now

image


Also, I've used eslint-plugin-deprecation to detected and remove deprecated APIs of rxjs and body-parser and ones related with Buffer (nodejs). The latter only touches .spec.ts files.

Does this PR introduce a breaking change?

  • Yes
  • No

Do notice that, basically:

  • before: obj is observable when isFunction(obj.subscribe)
  • now: obj is observable when isFunction(obj.subscribe) && isFunction(obj.lift)

I'm pretty sure this means that there are no breaking changes due to how isObservable is being used in this codebase but I could be wrong.

@coveralls
Copy link

coveralls commented Dec 24, 2021

Pull Request Test Coverage Report for Build d4447bdf-1f5d-4944-ba9e-8984ae03e7c3

  • 26 of 27 (96.3%) changed or added relevant lines in 9 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.003%) to 94.126%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/microservices/server/server-grpc.ts 2 3 66.67%
Totals Coverage Status
Change from base Build f821b4c8-5d49-4afa-9ee8-5265f6c6b09b: -0.003%
Covered Lines: 5689
Relevant Lines: 6044

💛 - Coveralls

Relying on `isObservable` type guard from RxJS is better as it has less
false-positives.
on every type assertion for `Observable<any>` that is not needed anymore.
@micalevisk
Copy link
Member Author

I didn't wrote missing tests for server-grpc.ts because I'm not familiar with gRPC.

image

Also, I didn't really understand the following

call.on('error', (e: any) => {
// Check if error means that stream ended on other end
const isCancelledError = String(e).toLowerCase().indexOf('cancelled');
if (isCancelledError) {
call.end();
return;
}
// If another error then just pass it along
req.error(e);
});

because if (isCancelledError) will be evaluate to true when e !== 'canceled', which doesn't make sense to me.

@micalevisk micalevisk changed the title Use isObervable from rxjs instead of our own implementation, and drop few deprecated APIs from 3rd-party libs Use isObervable from rxjs instead of our own implementation. Drop few deprecated APIs from 3rd-party libs and remove useless filtering operation Dec 24, 2021
@micalevisk micalevisk changed the title Use isObervable from rxjs instead of our own implementation. Drop few deprecated APIs from 3rd-party libs and remove useless filtering operation Use isObervable from rxjs instead of our own implementation. Drop few deprecated APIs from 3rd-party libs Dec 24, 2021
Replacie old and deprecated APIs from `rxjs`, `body-parser`
and NodeJS core (read this guide https://nodejs.org/en/docs/guides/buffer-constructor-deprecation).
Do notice that the later only touches test files, thus doesn't affect
production code.
Before this change, when supplying an observable to
`Server#transformToObservable` method, the return type was wrongly inferred as
`Observable<Observable<X>>` while the real return is
`Observable<X>`
This commits fix this return type.
Copy link
Member

@jmcdo29 jmcdo29 left a comment

Choose a reason for hiding this comment

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

Looks good to me

Also, add another test for `isString` utility, to clarify that it must
return `false` for strings made by `String` constructor.
@micalevisk micalevisk changed the title Use isObervable from rxjs instead of our own implementation. Drop few deprecated APIs from 3rd-party libs Use isObervable from rxjs instead of our own implementation. Drop few deprecated APIs from 3rd-party libs. And other minor refactorings Dec 26, 2021
@kamilmysliwiec kamilmysliwiec merged commit 28516d7 into nestjs:master Dec 27, 2021
@kamilmysliwiec
Copy link
Member

LGTM

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

Successfully merging this pull request may close these issues.

Not every object that has a subscribe method is an observable
4 participants