CodeSequence / jasmine-marbles

Marble testing helpers for RxJS and Jasmine
MIT License
117 stars 39 forks source link

chore: update RxJS dependencies to 7.x.x #67

Closed qdelettre closed 3 years ago

qdelettre commented 3 years ago

Hi,

Rxjs released 7.0.0 and 7.0.1 end of last month, here is the PR to update the dependency.

I'm not familiar with this repo, so i'm open to feedbacks as i see tests are failing :( I have not updated the readme to inform about supported rxjs versions.

brandonroberts commented 3 years ago

Thanks @qdelettre! It seems the items in the results vs expected no longer match here after the update. https://github.com/synapse-wireless-labs/jasmine-marbles/blob/master/index.ts#L155.

qdelettre commented 3 years ago

Yes, i can see that the notification properties are not equals. Ex for "should work with a cold observable"

Results object:

[
  {
    frame: 0,
    notification: Notification {
      kind: 'N',
      value: 1,
      error: undefined,
      hasValue: true
    }
  },
  {
    frame: 0,
    notification: Notification {
      kind: 'C',
      value: undefined,
      error: undefined,
      hasValue: false
    }
  }
]

Expected object:

[
  { frame: 0, notification: { kind: 'N', value: 1, error: undefined } },
  {
    frame: 0,
    notification: { kind: 'C', value: undefined, error: undefined }
  }
]

It is related to the notification creation. Btw in rxjs codebase, they do not recommand to create instances of Notification directly.

Changing the notification creation with pojo matching the signature of ObservableNotification makes all the tests pass locally. I updated the PR with these changes.

Build is still failling for deprecated property and interfaces in v7.

brandonroberts commented 3 years ago

You'll have to bump the TypeScript version to 4.2 also as it's a minimum for RxJS 7. We'll make it a breaking change and update the supported version ranges.

brandonroberts commented 3 years ago

Thanks @qdelettre! I'll cut a release for 0.9.0 soon