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

feat(tap): add assertion function overload #7272

Closed
wants to merge 1 commit into from

Conversation

Milly
Copy link

@Milly Milly commented May 26, 2023

Description:

I think no breaking changes in this PR.

Specifying an assertion function to tap() will ensure the type that follows.
This is similar to specifying a type guard function to filter().

const stringOrNumberObservable: Observable<string | number> = of(1, 'aaa', 3, 'bb');

// Keep type-inference before PR
stringOrNumberObservable
  .pipe(tap((x) => {
    if (typeof x !== 'string') {
      throw new Error('not a String');
    }
  }))
  .subscribe((x) => x); // x is still {string | number}

// New type-inference in this PR
stringOrNumberObservable
  .pipe(tap((x: string | number): asserts x is string => {
    if (typeof x !== 'string') {
      throw new Error('not a String');
    }
  }))
  .subscribe((s) => s.length); // s is {string}

Related issue (if exists):
Nothing.

ensure input type when throwing conditionally in `next` of `tap`
@dilame
Copy link

dilame commented Jun 9, 2023

I think tap operator is not the right choice for this type of job. tap is supposed to execute side effects only, thus, it can't change output type, which imply it to be MonotypeOperatorFunction

@Milly
Copy link
Author

Milly commented Jun 9, 2023

tap is supposed to execute side effects only

I see.
Anyone have any suggestions for a suitable name?
ensure is good?

@demensky
Copy link
Contributor

demensky commented Jun 9, 2023

In this case, it turns out that you will have to do overloads in all operators that have a callback. It will be much easier to just make an alias on a specific project:

function ensure<T, R extends T>(
  next: (value: T) => asserts value is R
): OperatorFunction<T, R> {
  return tap({ next }) as OperatorFunction<T, R>;
}
const stringOrNumberObservable = of(1, 'aaa', 3, 'bb');

stringOrNumberObservable
  .pipe(
    ensure((x): asserts x is string => {
      if (typeof x !== 'string') {
        throw new Error('not a String');
      }
    })
  )
  .subscribe((s) => s.length); // s is {string}

@dilame
Copy link

dilame commented Jun 10, 2023

To be honest, I don't see a compelling reason to add such functionality to RxJS. The purpose of the asserts keyword is tied to local control flow when writing imperative code. Essentially, what asserts does is signal that the next computation will not execute if the variable is not of the expected type. For instance:

function assertIsString(value: unknown): asserts value is string {
  if (typeof value !== 'string') throw Error('value is not a string');
}

const x = 5;

assertIsString(x);
// This code is unreachable
console.log(x)

That is the only valid use case. In functional programming, different control flows exist within different functions, hence asserts x is string and x is string hold no distinction. RxJS already offers the filter() operator which supports type guards. If add asserts functionality, it brings no additional information.

The primary advantage of the asserts keyword is its influence on TypeScript's control flow analysis, which is particularly useful in imperative code, but in the context of RxJS and reactive programming, this approach isn't as directly applicable.

@benlesh
Copy link
Member

benlesh commented Dec 19, 2023

Thank you so much for the work, @Milly! This is an interesting idea, but I tend to agree with the statements above. Assertions like this are generally a part of imperative control flow... Hover, using that same concept, you can leverage map to narrow the type without any fancy typegaurd is statement:

declare const source$: Observable<any>;

const result: Observable<string> = source$.pipe(
  map(x => {
    if (typeof x !== 'string') {
      throw new TypeError('value must be a string!');
    }
    return x;
  })
);

@benlesh benlesh closed this Dec 19, 2023
@benlesh
Copy link
Member

benlesh commented Dec 19, 2023

Also, @Milly, if you disagree with this (and I know it took me forever to respond), please feel free to post concerns or whatever here.

An assert type operator might even be something @cartant would consider adding to rxjs-misc? I dunno. It's a cool idea, I'm just not sure that tap is the right place for it. tap is for side-effects, not really type-narrowing.

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