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

fix: fix up notification types #5478

Merged
merged 2 commits into from
Jun 23, 2020
Merged

Conversation

benlesh
Copy link
Member

@benlesh benlesh commented Jun 9, 2020

  • Ensures Notification methods only accept appropriate arguments
  • Ensures Arbitrary objects of the proper shape my be passed through dematerialized
  • Utilizes hot path functions to create lightweight objects for internal notification use
  • Deprecates the Notification class
  • Adds a deprecation message for materialize to notify users that soon the emitted object type will change to not have all of the same methods as Notification
  • Updates a few tests that were relying on the shape of Notification instances to pass, as we are now using POJOs internally in the TestScheduler
  • Adds dtslint tests for notifications
  • Adds dtslint tests to enforce types on dematerialize

BREAKING CHANGE: Notification.createNext(undefined) will no longer return the exact same reference everytime.

BREAKING CHANGE: Type signatures tightened up around Notification and dematerialize

spec-dtslint/Notification-spec.ts Outdated Show resolved Hide resolved
src/internal/Notification.ts Outdated Show resolved Hide resolved
src/internal/Notification.ts Show resolved Hide resolved
src/internal/Notification.ts Outdated Show resolved Hide resolved
src/internal/operators/dematerialize.ts Outdated Show resolved Hide resolved
src/internal/Notification.ts Outdated Show resolved Hide resolved
src/internal/Notification.ts Outdated Show resolved Hide resolved
src/internal/Notification.ts Show resolved Hide resolved
@benlesh benlesh requested a review from cartant June 15, 2020 23:01
Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

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

LGTM, but there's an assertion with an any that could be a narrower type. Approving this so it doesn't need to be re-reviewed, etc.

* @internal
*/
export function errorNotification(error: any): ErrorNotification {
return createNotification('E', undefined, error) as any;
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's one any left that could instead be an assertion (and the function could have an implicit return type).

- Ensures Notification methods only accept appropriate arguments
- Ensures Arbitrary objects of the proper shape my be passed through dematerialized
- Utilizes hot path functions to create lightweight objects for internal notification use
- Deprecates the Notification class
- Adds a deprecation message for materialize to notify users that soon the emitted object type will change to not have all of the same methods as Notification
- Updates a few tests that were relying on the shape of Notification instances to pass, as we are now using POJOs internally in the TestScheduler
- Adds dtslint tests for notifications
- Adds dtslint tests to enforce types on dematerialize

BREAKING CHANGE: Notification.createNext(undefined) will no longer return the exact same reference everytime.

BREAKING CHANGE: Type signatures tightened up around Notification and dematerialize
@benlesh benlesh merged commit 96868ac into ReactiveX:master Jun 23, 2020
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.

2 participants