Skip to content

feat: adding notification operator #575

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Feb 10, 2021
Merged

feat: adding notification operator #575

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
df596ae
feat: adding notification operator
anandtiwary Feb 9, 2021
2f8ea8b
Merge branch 'main' into notification-update
anandtiwary Feb 10, 2021
f554ee6
refactor: addressing comments
anandtiwary Feb 10, 2021
6887a19
Merge branch 'notification-update' of github.com:hypertrace/hypertrac…
anandtiwary Feb 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions projects/components/src/notification/notification.service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { MatSnackBar } from '@angular/material/snack-bar';
import { createServiceFactory, mockProvider, SpectatorService } from '@ngneat/spectator/jest';
import { of, throwError } from 'rxjs';
import { NotificationComponent, NotificationMode } from './notification.component';
import { NotificationService } from './notification.service';

Expand Down Expand Up @@ -55,4 +56,60 @@ describe('NotificationService', () => {
})
);
});

test('withNotification should work correctly', () => {
spectator = createService();

spectator.service.withNotification(of(true), 'success', 'failure').subscribe();

expect(spectator.inject(MatSnackBar).openFromComponent).toHaveBeenLastCalledWith(
NotificationComponent,
expect.objectContaining({
horizontalPosition: 'left',
verticalPosition: 'bottom',
duration: 5000,
data: expect.objectContaining({ message: 'success', mode: NotificationMode.Success })
})
);

spectator.service.withNotification(throwError('error'), 'success', 'failure').subscribe();

expect(spectator.inject(MatSnackBar).openFromComponent).toHaveBeenLastCalledWith(
NotificationComponent,
expect.objectContaining({
horizontalPosition: 'left',
verticalPosition: 'bottom',
duration: 0,
data: expect.objectContaining({ message: 'failure', mode: NotificationMode.Failure })
})
);
});

test('withNotification operator should work correctly', () => {
spectator = createService();

of(true).pipe(spectator.service.withNotificationOperator('success', 'failure')).subscribe();

expect(spectator.inject(MatSnackBar).openFromComponent).toHaveBeenLastCalledWith(
NotificationComponent,
expect.objectContaining({
horizontalPosition: 'left',
verticalPosition: 'bottom',
duration: 5000,
data: expect.objectContaining({ message: 'success', mode: NotificationMode.Success })
})
);

throwError('error').pipe(spectator.service.withNotificationOperator('success', 'failure')).subscribe();

expect(spectator.inject(MatSnackBar).openFromComponent).toHaveBeenLastCalledWith(
NotificationComponent,
expect.objectContaining({
horizontalPosition: 'left',
verticalPosition: 'bottom',
duration: 0,
data: expect.objectContaining({ message: 'failure', mode: NotificationMode.Failure })
})
);
});
});
20 changes: 19 additions & 1 deletion projects/components/src/notification/notification.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Injectable } from '@angular/core';
import { MatSnackBar, MatSnackBarConfig, MatSnackBarRef } from '@angular/material/snack-bar';
import { EMPTY, Observable, Subject } from 'rxjs';
import { EMPTY, noop, Observable, Subject } from 'rxjs';
import { tap } from 'rxjs/operators';
import { NotificationComponent, NotificationMode } from './notification.component';
import { NotificationModule } from './notification.module';

Expand Down Expand Up @@ -48,4 +49,21 @@ export class NotificationService {
data: { message: message, mode: NotificationMode.Info, closedObserver: this.closedObserver$ }
});
}

public withNotification<T>(source: Observable<T>, successMessage: string, failureMessage: string): Observable<T> {
return source.pipe(
tap(
Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Feb 10, 2021

Choose a reason for hiding this comment

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

How difficult is it to make a real operator (I've done it in rxjava where it's pretty confusing, haven't looked in rxjs)? My concern with this one is that it gets awkward to use in a pipe

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Feb 10, 2021

Choose a reason for hiding this comment

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

Oh I take it back - it looks like rxjs operators are much simpler (should have read your other docs first). Given that, I may suggest switching the names around a bit (so the name looks more like an operator) but otherwise looks good.

something like withNotification -> wrapWithNotification and withNotificationOperator -> withNotification

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Feb 10, 2021

Choose a reason for hiding this comment

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

For comparison, this is what an operator in java looked like 🙄
https://github.com/hypertrace/query-service/blob/main/query-service-impl/src/main/java/org/hypertrace/core/query/service/RowChunkingOperator.java

Copy link
Contributor Author

@anandtiwary anandtiwary Feb 10, 2021

Choose a reason for hiding this comment

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

Yea I tried building our own operator. I got it to work too, but realized that it still has to go via notification service (since we need injected service). With that in consideration, the native implementation was not providing much value. I switched to the current approach which anyway goes via the service but is much more readable and maintainable.

Copy link
Contributor Author

@anandtiwary anandtiwary Feb 10, 2021

Choose a reason for hiding this comment

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

Will update the name

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Feb 10, 2021

Choose a reason for hiding this comment

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

Ah right - so looking at an example, pipe takes a function of UnaryFunction<Observable<T>, Observable<R>> but the standard lib ones are generally implemented via lift and an operator. I'll have to page back in if there's any benefit to using lift + operator vs pipe, but nothing seems obvious (and if so we can do it without changing the pai.

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Feb 10, 2021

Choose a reason for hiding this comment

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

Even their docs examples use pipe - I'd guess lift is the old way of doing it (before they used pipe when everything was on the prototype)
https://reactive.how/rxjs/pipeable-operators

Copy link
Contributor Author

@anandtiwary anandtiwary Feb 10, 2021

Choose a reason for hiding this comment

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

Yeah, the pipe approach is pretty popular. rxjs folks are also adding newer apis in 7.x, so this could change too.

The lift approach would require us to build our own operator which comes with additional code.

I think if we are doing something pretty involved then building our own operator makes sense. For something simple, Piggybacking on existing
operators is much cleaner fnow.

noop,
() => this.createFailureToast(failureMessage),
() => this.createSuccessToast(successMessage)
)
);
}

public withNotificationOperator<T>(
successMessage: string,
failureMessage: string
): (source: Observable<T>) => Observable<T> {
return (source: Observable<T>) => this.withNotification(source, successMessage, failureMessage);
}
}