RxJS V6 Support #78

johnlindquist · 2018-03-29T20:33:31Z

RxJS v6 now uses a "pipeable" operators paradigm thus changing how fromEvent and share are required when installing.

Plugin Installation API

API-wise:
Vue.use(VueRx, {Observable, Subject, Subscription})
now requires fromEvent and share
Vue.use(VueRx, {Observable, Subject, Subscription, fromEvent, share})

stream.js and createObservableMethod.js were updated to support either API.

Tests

Tests were separated into v5.test.js and v6.test.js.
v5 now uses rxjs-compat for backwards compatibility.
** Note a small breaking change with errors: https://twitter.com/johnlindquist/status/979408339858845704
v6 are the same tests, but using rxjs and "pipeable" operators

… if on Observable or require User to pass them in to plugin config due to v6's breaking changes. Separated tests into v5 and v6.

benlesh

Overall, I'm confused by what seems to be a use of UMD globals for RxJS, there's a LOT of bundle size that will be lost from this choice, but that doesn't appear to have anything to do with this PR, per se.

I'd recommend using Webpack 4 and RxJS 6 without compat if possible to get the smallest output bundle size possible.

benlesh · 2018-03-29T22:30:52Z

package.json

@@ -43,7 +43,8 @@
    "rollup": "^0.50.0",
    "rollup-plugin-buble": "^0.16.0",
    "rollup-watch": "^4.3.1",
-    "rxjs": "^5.2.0",
+    "rxjs": "^6.0.0-beta.3",
+    "rxjs-compat": "^6.0.0-beta.1",


rxjs-compat and rxjs should be the same version here. both are at beta.4 right now.

benlesh · 2018-03-29T22:32:53Z

src/directives/stream.js

@@ -45,7 +45,8 @@ export default {
        })
      })
    } else {
-      if (!Rx.Observable.fromEvent) {
+      const fromEvent = Rx.fromEvent || Rx.Observable.fromEvent


This is apparently using the umd global?

All locations in the v6 global are a mirror of the module locations, only with periods. This should be:

const fromEvent = rxjs.fromEvent

This library passes in it's own Rx that comes from the configuration, so Rx is valid here.

benlesh · 2018-03-29T22:34:02Z

src/methods/createObservableMethod.js

@@ -13,7 +13,8 @@ export default function createObservableMethod (methodName, passContext) {
  }
  const vm = this

-  if (!Rx.Observable.prototype.share) {
+  const share = Rx.share || Rx.Observable.prototype.share
+  if (!share) {


const share = rxjs.operators.share if we're using the umd global

same as above

benlesh · 2018-03-29T22:34:20Z

src/methods/createObservableMethod.js

@@ -52,5 +53,8 @@ export default function createObservableMethod (methodName, passContext) {
  }

  // Must be a hot stream otherwise function context may overwrite over and over again
+  if (Rx.share) {


rxjs.operators.share

same as above

benlesh · 2018-03-29T22:34:35Z

src/methods/createObservableMethod.js

@@ -52,5 +53,8 @@ export default function createObservableMethod (methodName, passContext) {
  }

  // Must be a hot stream otherwise function context may overwrite over and over again
+  if (Rx.share) {
+    return Rx.Observable.create(creator).pipe(share())
+  }


rxjs.Observable

All the Rx references are an object passed into the plugin, not the actual Rx package (I know that's confusing). Take a look at how install works:

https://github.com/vuejs/vue-rx/blob/master/src/util.js#L5

benlesh · 2018-03-29T22:35:26Z

test/v5.spec.js

-          expect(param).toEqual('hola')
-          done()
-        })
+      this.$createObservableMethod('add').subscribe(function (param) {


Was the formatting change necessary?

regou

Why format so many test code😢

regou · 2018-03-30T11:45:20Z

Reviewed

@yyx990803 Basically, v6 support pipe operators. Users may want import each individual operators (not on Rx.Observable as used do on v5).
Major version update ✅ needed ( beta? )
Bundle size reduce codesnipts 👆 ‘Plugin Installation API’ update ⚠️ needed

johnlindquist · 2018-03-30T12:51:03Z

@regou sorry about the formatting. I ran the linter and had the editor config plugin running as well... I can manually fix it if you'd like.

mistyharsh · 2018-03-31T05:25:17Z

@johnlindquist, I believe Rxjs v6 RC is out - 6.0.0-rc.0.

coun7zero · 2018-04-25T11:02:00Z

RxJS 6 is out! Maybe it's time to pass that pull request... ;)

yyx990803 · 2018-04-25T12:53:41Z

@johnlindquist can you update this PR to use official RxJS 6?

david-driscoll · 2018-05-09T05:23:03Z

@johnlindquist you should probably update the readme, with the requirements of share and fromEvent

david-driscoll · 2018-05-12T15:26:38Z

I'm doing some testing with this locally (with rxjs6), and running into a fun issue. Since the types.d.ts imports rxjs/Observable, this triggers a dependency on rxjs-compat.

This means that the types can only really work for rxjs6 or rxjs5...

I think the best solution to this is to just copy the required interfaces from rxjs, so that the library isn't coupled to either version.

Thoughts?

My current .d.ts types looks like...

import Vue from 'vue'
import { WatchOptions } from 'vue'

export interface Unsubscribable {
  unsubscribe(): void;
}

export interface Subscribable<T> {
  subscribe(observerOrNext?: PartialObserver<T> | ((value: T) => void),
            error?: (error: any) => void,
            complete?: () => void): Unsubscribable;
}

export interface NextObserver<T> {
  closed?: boolean;
  next: (value: T) => void;
  error?: (err: any) => void;
  complete?: () => void;
}

export interface ErrorObserver<T> {
  closed?: boolean;
  next?: (value: T) => void;
  error: (err: any) => void;
  complete?: () => void;
}

export interface CompletionObserver<T> {
  closed?: boolean;
  next?: (value: T) => void;
  error?: (err: any) => void;
  complete: () => void;
}

export type PartialObserver<T> = NextObserver<T> | ErrorObserver<T> | CompletionObserver<T>;

export type Observables = Record<string, Subscribable<any>>
declare module 'vue/types/options' {
  interface ComponentOptions<V extends Vue> {
    subscriptions?: Observables | ((this: V) => Observables)
    domStreams?: string[]
    observableMethods?: string[] | Record<string, string>
  }
}

export interface WatchObservable<T> {
  newValue: T
  oldValue: T
}
declare module "vue/types/vue" {
  interface Vue {
    $observables: Observables;
    $watchAsObservable(expr: string, options?: WatchOptions): Subscribable<WatchObservable<any>>
    $watchAsObservable<T>(fn: (this: this) => T, options?: WatchOptions): Subscribable<WatchObservable<T>>
    $eventToObservable(event: string): Subscribable<{name: string, msg: any}>
    $subscribeTo<T>(
      observable: Subscribable<T>,
      next: (t: T) => void,
      error?: (e: any) => void,
      complete?: () => void): void
    $fromDOMEvent(selector: string | null, event: string): Subscribable<Event>
    $createObservableMethod(methodName: string): Subscribable<any>
  }
}

export declare function install(V: typeof Vue): void

pinghe · 2018-05-15T02:54:34Z

rxjs5:

import VueRx from 'vue-rx'

import { Observable } from 'rxjs/Observable'
import { Subject } from 'rxjs/Subject'
import { fromEvent } from 'rxjs/fromEvent'

Vue.use(VueRx, {
  Observable,
  Subject,
  fromEvent
})

rxjs6

import VueRx from 'vue-rx'

import { Observable, Subject, fromEvent } from 'rxjs'

Vue.use(VueRx, {
  Observable,
  Subject,
  fromEvent
})

yyx990803 · 2018-05-22T21:59:35Z

Since I'm not a frequent RxJS user, a few questions for everyone in this thread:

Is being compatible with v5 and v6 at the same time really necessary? If a user needs v5 they can continue to use 5.x of vue-rx. This should get rid of the typing problem @david-driscoll ran into.
One thing I'm a bit confused about is the fact that when you import operators from rxjs/operators according to the migration guide, it still goes to rxjs-compat. Does this mean RxJS v6 is not quite ready yet?

Ideally, we probably want to do a new build setup where we export two builds:

A UMD build where it expects Rx to be installed (like current)
An ESM build where it assumes a ES-capable module bundler. In this build we can directly import the things needed and the user can just Vue.use(VueRx).

benlesh · 2018-05-23T00:05:33Z

Is being compatible with v5 and v6 at the same time really necessary?

@yyx990803 It can be... however, what a library should do that's using RxJS is just use v6. Then it's up to the consumer of that library to include rxjs-compat if they need backward compatibility for v5 code they have.

One thing I'm a bit confused about is the fact that when you import operators from rxjs/operators according to the migration guide, it still goes to rxjs-compat. Does this mean RxJS v6 is not quite ready yet?

v6 is totally ready.. the only reason that importing would go into rxjs-compat is if you imported from a deprecated path underneath rxjs/operators/**.

Again, rxjs-compat isn't a requirement. It's just there for people who want to delay migrating their client code to v6. If you have a library (such as this) that is using RxJS, I'd recommend just updating to rxjs@6 and not including rxjs-compat. Leave that to your clients to decide.

RxJS 6 is a win, especially when combined with Webpack 4, and it's setting up for v7 which will be massively reduced in size and much faster overall.

benlesh · 2018-05-23T00:06:44Z

package.json

@@ -43,14 +36,13 @@
    "rollup": "^0.50.0",
    "rollup-plugin-buble": "^0.16.0",
    "rollup-watch": "^4.3.1",
-    "rxjs": "^5.2.0",
+    "rxjs": "^6.0.0",
+    "rxjs-compat": "^6.0.0",


Probably don't want this included by default, leave that to consumers of the library to decide whether they need it or not.

These are dev deps

benlesh · 2018-05-23T00:08:09Z

test/v5.spec.js

-const Subscription = require('rxjs/Subscription').Subscription
-require('rxjs/add/observable/fromEvent')
-require('rxjs/add/operator/share')
+const Observable = require('rxjs-compat/Observable').Observable


I would import from the proper locations expected in v6... other rxjs or rxjs/operators. Not from rxjs-compat. That should never be imported from directly anyhow.

benlesh · 2018-05-23T00:09:18Z

test/v5.spec.js

@@ -151,7 +153,8 @@ test('v-stream directive (basic)', done => {
    domStreams: ['click$'],
    subscriptions () {
      return {
-        count: this.click$.map(() => 1)
+        count: this.click$
+          .map(() => 1)


All operator use should be convered to "pipeable" operators:

this.click$.pipe( map(() => 1), .... )

david-driscoll · 2018-05-23T15:40:34Z

The underlying problem is that the current typings will require rxjs-compat to be installed. If we change the typings to use the rxjs6 then the types become invalid for rxjs5.

Personally I'm fine saying just use rxjs6 however I know this library is designed to support more than just the latest rxjs. Hence my typing changes above. Those typing changes just mirror the interfaces for rxjs5/6 which have not changed from 5 to 6. (We were breaking out the observable patching for purposes of tree shaking, etc).

My solution solves the issue for both current consumers and allows new consumes to strictly target just rxjs6.

benlesh · 2018-05-23T17:14:52Z

If we change the typings to use the rxjs6 then the types become invalid for rxjs5.

That shouldn't at all be the case. This is precisesly what rxjs-compat was designed for.

Case in point: Angular v6 is using rxjs v6 exclusively, and apps with rxjs 5 code can "just work" if they install rxjs-compat.

david-driscoll · 2018-05-23T17:25:49Z

The meaning of the rxjs module has changed between the two.

For rxjs5:
If someone is using specific operator overloads, then we change the import to import { Observable } from 'rxjs' suddenly they'll have all the operators imported through the type system. However they may have only really imported 10 operators to the Observable.prototype, so then suddenly they'll get errors for methods that were never explicitly imported. Yes it functionally works but it most definitely not intended.

For rxjs6:
If we leave it as is import { Observable } from 'rxjs/Observable' then we get an error requiring rxjs-compat to be installed because rxjs/Observable contains the compat shim.

If we copy Unsubscribable, Subscribable, and Observer interfaces, then we can support both because the type system doesn't care if the interfaces are different as long as they are equivalent. (this is different for classes and some of that has been fixed)

benlesh · 2018-05-24T14:47:24Z

@david-driscoll is there something that needs fixed on our end? Because the general idea is that libraries and frameworks should be using rxjs, and people that need it should use rxjs-compat. Maybe we should take this discussion to rxjs slack...

For this issue, my advice stands, Vue, Angular, et al, should be using rxjs@6 and their consumers should use rxjs-compat only if necessary because they aren't ready to update their rxjs 5 code yet (or because they have a dependency that requires v 5)

david-driscoll · 2018-05-25T16:11:53Z

Nothing on the rxjs side can really be fixed because of the way the new imports are different between the two. Unless we stopped requiring rxjs-compat for rxjs/Observable and friends, which is not something we want to do for 6+ imo.

The changes here I'm proposing are just for the typings in this library, so that it can work with rxjs5 and rxjs6 without causing a headache for either user.

benlesh · 2018-05-25T22:27:57Z

@david-driscoll if this library just uses v6, all consumers will be able to use v5 if they want by simply including rxjs-compat in their app. There aren't any types to fix here.

david-driscoll · 2018-05-27T14:51:25Z

The imports currently are:

vue-rx/types/index.d.ts

Lines 1 to 3 in 44dfefd

    
           import Vue from 'vue' 
        
           import { WatchOptions } from 'vue' 
        
           import { Observable } from 'rxjs/Observable'

The type declarations for rxjs/Observable are currently export * from 'rxjs-compat/Observable'; in rxjs6.
In rxjs 5, this was the the actual Observable class itself. However the import of rxjs in rxjs5 imported "all the things!"

This library however does not import rxjs at all by itself, instead you pass it an object that includes a thing that is named Observable, Subject, etc. They don't have to be a specific rxjs verison. This allows it to be used for any version of rx or something else like most, assuming the behaviors of those named things are similar.

So now when I install this library with rxjs6.

import Vue from 'vue';
import VueRx from 'vue-rx';
import { fromEvent, Observable, Subject, Subscription } from 'rxjs';
import { share } from 'rxjs/operators';

Vue.use(VueRx as any, { Observable, Subject, Subscription, fromEvent, share });

When the types get imported they use

vue-rx/types/index.d.ts

Lines 1 to 3 in 44dfefd

    
           import Vue from 'vue' 
        
           import { WatchOptions } from 'vue' 
        
           import { Observable } from 'rxjs/Observable'

Which then forces me into installing rxjs-compat, even though I'm strictly using rxjs6.

If we reverse things and want to use rxjs5 (or are an existing user), and the types are updated to strictly use import {Subject, Observable, Subscription} from 'rxjs'.

import Vue from 'vue';
import VueRx from 'vue-rx';
import { Observable } from 'rxjs/Observable'
import { Subscription } from 'rxjs/Subscription' // Disposable if using RxJS4
import { Subject } from 'rxjs/Subject' // required for domStreams option

Vue.use(VueRx as any, { Observable, Subject, Subscription });

Now my type system has been polluted with types that I have not explicitly imported, so I'll start seeing map, filter, etc on my Observable.prototype, and I'll start running into undefined is not a function for any operator I haven't explicitly added.

david-driscoll · 2018-05-27T14:53:02Z

That is why I proposed bringing in the types with the comment above: #78 (comment)

benlesh · 2018-05-30T16:18:55Z

The imports should be updated to be import { Observable } from 'rxjs'

benlesh · 2018-05-30T17:17:24Z

Okay, I've submitted a separate PR that follows what I'd recommend doing here. (Sorry, @johnlindquist)

It's got breaking changes, so if you decide you want to go with it, you'd want to move up a major version.

See #84

yyx990803 · 2018-05-30T20:26:34Z

Closing in favor of #84 - but thanks for getting this off the ground @johnlindquist !

johnlindquist added 9 commits March 29, 2018 13:44

Supporting RxJS v6 (still supports v5). "fromEvent" and "share" check…

9bd4f76

… if on Observable or require User to pass them in to plugin config due to v6's breaking changes. Separated tests into v5 and v6.

removing stray character 👀

65dec96

fixing a typecheck error (missing iterable and symbol in RxJS)

8abeeff

fixing formatting

bcd7a45

fixing formatting

05be809

formatting

7dbe408

formatting

2e6f983

formatting

f79aabe

formatting

eec0df2

benlesh suggested changes Mar 29, 2018

View reviewed changes

benlesh mentioned this pull request Mar 29, 2018

This should not be using the UMD/Global version of RxJS #79

Closed

updating to rxjs and compat to beta.4

bc93337

regou approved these changes Mar 30, 2018

View reviewed changes

updating rxjs to stable v6

ae5f423

regou approved these changes May 9, 2018

View reviewed changes

pinghe approved these changes May 9, 2018

View reviewed changes

regou assigned yyx990803 May 18, 2018

benlesh suggested changes May 23, 2018

View reviewed changes

yyx990803 closed this May 30, 2018

RxJS V6 Support #78

RxJS V6 Support #78

Conversation

johnlindquist commented Mar 29, 2018

Plugin Installation API

Tests

benlesh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

regou left a comment

Choose a reason for hiding this comment

regou commented Mar 30, 2018 • edited Loading

johnlindquist commented Mar 30, 2018

mistyharsh commented Mar 31, 2018

coun7zero commented Apr 25, 2018

yyx990803 commented Apr 25, 2018

david-driscoll commented May 9, 2018

david-driscoll commented May 12, 2018

pinghe commented May 15, 2018

yyx990803 commented May 22, 2018

benlesh commented May 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-driscoll commented May 23, 2018

benlesh commented May 23, 2018

david-driscoll commented May 23, 2018

benlesh commented May 24, 2018

david-driscoll commented May 25, 2018

benlesh commented May 25, 2018

david-driscoll commented May 27, 2018

david-driscoll commented May 27, 2018

benlesh commented May 30, 2018

benlesh commented May 30, 2018

yyx990803 commented May 30, 2018

regou commented Mar 30, 2018 •

edited

Loading

benlesh commented May 23, 2018 •

edited

Loading