rewrite links with rxjs

[sachinraja]

closes #1420

Switch from the trpc rxjs implementation to actual rxjs.

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

next-prisma-starter – ./examples/next-prisma-starter

🔍 Inspect: https://vercel.com/trpc/next-prisma-starter/ANxzWVN4dyxes24pbCfhmySbA2i2
✅ Preview: https://next-prisma-starter-git-feature-rxjs-trpc.vercel.app

www – ./www

🔍 Inspect: https://vercel.com/trpc/www/77K9UQNNFBwwre5iUAhvU3UKcZRx
✅ Preview: https://www-git-feature-rxjs-trpc.vercel.app

[KATT]

Thanks, @sachinraja! ❤️

I sent you another $100 - this was not by any means an easy rewrite and I'm super impressed by your work. The only thing I fixed was in d33319f -- this in JS unfortunately sucks and it's this stuff is the main reason why I try to avoid JS classes at all cost and hence a reason why I didn't want to use RxJS in the first place (but that's really a criticism of the spec and not RxJS itself).

@benlesh will likely be very happy with this, feel free to review if you feel inclined to :)

[KATT]

@benlesh - the only things bogging me now:

• Errors are untyped any
• When calling .unsubscribe() I'd expected/want my complete() to be called and this is why one of the tests is failing - what is the concept I'm missing here? I'm sure this is a common misconception and I'm not finding the right thing in the docs

[sachinraja]

Thank you so much @KATT!

this in JS unfortunately sucks and it's this stuff is the main reason why I try to avoid JS classes at all cost

Well that's annoying, nice catch on that.

One thing that might be improved in the current implementation is a replacement for observableToPromise.

[benlesh]

I'm wondering if there's a few small wins that could be had. For example, rxjs's share operator is kind of a "one share to rule them all" sort of solution that also includes Subject and some other things you're not necessarily using, and it might be that your existing share operator would work fine, and you don't need RxJS's copy.

[benlesh]

Just as a talking point: I created an angular-cli app and installed rxjs@7.5.5, then only included the bits you were using (Observable, map, tap, and share), and even including the stuff that @angular/router et al brings in - The total size of RxJS in the bundle was ~13.7kB. So seems unlikely there could be a size increase greater than that. Especially given that multiple files were deleted from this library.

Something feels off about the other measurements, and I'd love to be able to take the same measurements to see what the differences are.

[sachinraja]

@benlesh got a visual with rollup at https://github.com/sachinraja/rxjs-rollup-bundle.

Note that this is unminified. Minified via terser, it's ~20 KB.

Also the angular webpack config is resolving the es2015 export condition but most bundler setups will not do that, although they would have resolved an import condition if there was one. When I setup rollup to resolve the es2015 condition and minify, it's ~15 KB, much closer to what angular produces. It seems like we could further reduce this by removing tslib and trpc using its own share implementation like you said.

[benlesh]

So that called out a bug in our code that was including the wrong function and requiring a bunch of unnecessary stuff. Fixing that reduces the size another 5kB in your test.

HOWEVER.. by excluding share, we can remove an additional ~18kb.

But that means we still need a lightweight share, which I was able to roll in less than 1K:

[benlesh]

For reference, the "share light" is what's below. I didn't test it or anything, it's just a simple implementation, and I cheat a bit and used an internal function (that you're not supposed to use, but I know isn't going to change in version 7)

import { operate } from "rxjs/internal/util/lift";

export function share() {
  const subscribers = new Set();
  let connection = null;
  return operate((source, subscriber) => {
    subscribers.add(subscriber);
    subscriber.add(() => {
      subscribers.delete(subscriber);
      if (subscribers.size === 0) {
        connection.unsubscribe();
        connection = null;
      }
    });
    if (!connection) {
      connection = source.subscribe({
        next: (value) => subscribers.forEach(s => s.next(value)),
        error: (error) => subscribers.forEach(s => s.error(error)),
        complete: () => subscribers.forEach(s => scomplete()),
      });
      connection.add(() => subscribers.clear() );
    }
  });
}

[sachinraja]

Wow that is very helpful. Great job reducing the bundle size. I just tried switching out for the lighter share and it looks good, but we can't reach into any of the rxjs internal functions because there are no cjs paths for it (like how there is a folder with package.json in it for rxjs/operators). So it will only work in environments that understand package.json exports.

[benlesh]

It can be built as a ordinary operator too, feel free to not use that internal function, It's just a small wrapper around (source) => new Observable().

I'm very grateful for this thread. It pointed out a flaw that was easily fixed. RxJS will only get smaller as time goes on.

[benlesh]

@trpc/client@10.0.0-alpha.9: 17.6 kB -> 5.23 kB
@trpc/client@10.0.0-alpha.10: 175 kB -> 39.3 kB

FYI: I think that the numbers from bundle.js.org can be dismissed. It seems it's not properly tree-shaking the rxjs dependency from what you're using, as it looks like it's including literally everything from rxjs (147 kB -> 32.8 kB)

So instead of including 44kB from rxjs (still not amazing) it was including all 147kB for whatever reason.

I feel that if you take the advice above, you should be able to land RxJS and not see significant bundle size changes. The win mostly being that you don't have to maintain your own Observable type, and anyone that happens to be using RxJS won't have to pay the cost of your Observable on top of theirs.

In any case, this has been a hugely interesting thing to watch/help with.

All of that said. If you're truly being size/performance conscious, I'd recommend removing any abstractions like this at all from your internals entirely. It can be more bug prone and difficult to write certain things that way, but if the goal is speed and size, that's probably the way to go. If you find yourself wanting to expose an "observable contract" of sorts to your users, there are small/light ways to do that that can interop well with RxJS et al. (Symbol.observable use is fairly widespread, for example)

[sachinraja]

FYI: I think that the numbers from bundle.js.org can be dismissed. It seems it's not properly tree-shaking the rxjs dependency from what you're using, as it looks like it's including literally everything from rxjs (147 kB -> 32.8 kB)

There does seem to be an issue with it. I believe 32 KB might actually be the bundled+minified+gzipped size rather than the bundled+minified size so it's not comparable with our other experiments either.

All of that said. If you're truly being size/performance conscious, I'd recommend removing any abstractions like this at all from your internals entirely. It can be more bug prone and difficult to write certain things that way, but if the goal is speed and size, that's probably the way to go

I think this is something we should definitely consider. I'm also not sure if rxjs is the best solution or way to model links either. @KATT is this something you're interested in discussing?

[benlesh]

FWIW: I'm willing to help to some degree with any of this. I will say though, that if you're exporting any type that is supposed to push streaming updates, I highly recommend having it adhere to an observable contract (which is simple enough to implement in a naive way). Basically:

class ObservableLike<T> {
   subscribe(observer: Partial<Observer<T>>): Unsubscribable {
     // setup here 
     // Just be careful:
     // 1. don't call non-existant handlers
     // 2. don't call handlers out of order.
     // 3. Make sure you teardown everything after error or complete!
     
     // return a "subscription" for consumers to cancel with.
     return {
        unsubscribe: () => {
            // teardown here
        }
     }
     
     // Now it's compatible with RxJS and dozens of other libraries
     [Symbol.observable ?? '@@observable']() {
       return this;
     }
   }
}

RxJS observable (or any "good" observable implementation) will provide all of the safety for you. So you need to be careful. But you'll have to be careful if you're doing anything like this with callbacks, too.

[sachinraja]

@KATT can you do another publish so everyone can check the new bundle size? I updated it with the lighter share.

I think we should move ahead with rxjs and consider using a generic Observable later.

[KATT]

Sorry for being off here.

@KATT can you do another publish so everyone can check the new bundle size? I updated it with the lighter share.

Done!

I think we should move ahead with rxjs and consider using a generic Observable later.

It didn't do much difference.... I really find it hard to stomach a multiple X increase in bundle size when we already have an implementation that is on par for what we're doing.

I think what would make sense is that we'd have interoperability with RxJS in our own implementation, so we can easily swap in the future once RxJS has gotten smaller.

[sachinraja]

superseded by #1644

[github-actions]

This pull request has been locked because it had no new activity for 30 days. If you think, this PR is still necessary, create a new one with the same branch. Thank you.