-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
rewrite links with rxjs #1592
rewrite links with rxjs #1592
Conversation
This pull request is being automatically deployed with Vercel (learn more). next-prisma-starter – ./examples/next-prisma-starter🔍 Inspect: https://vercel.com/trpc/next-prisma-starter/ANxzWVN4dyxes24pbCfhmySbA2i2 www – ./www🔍 Inspect: https://vercel.com/trpc/www/77K9UQNNFBwwre5iUAhvU3UKcZRx |
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 -- @benlesh will likely be very happy with this, feel free to review if you feel inclined to :) |
@benlesh - the only things bogging me now:
|
Thank you so much @KATT!
Well that's annoying, nice catch on that. One thing that might be improved in the current implementation is a replacement for |
export type UnsubscribeFn = () => void;
I'm wondering if there's a few small wins that could be had. For example, rxjs's |
Just as a talking point: I created an 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. |
@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 |
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 But that means we still need a lightweight |
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() );
}
});
} |
Wow that is very helpful. Great job reducing the bundle size. I just tried switching out for the lighter |
It can be built as a ordinary operator too, feel free to not use that internal function, It's just a small wrapper around 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. |
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. ( |
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.
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? |
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. |
@KATT can you do another publish so everyone can check the new bundle size? I updated it with the lighter I think we should move ahead with rxjs and consider using a generic Observable later. |
Sorry for being off here.
Done!
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. |
superseded by #1644 |
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. |
closes #1420
Switch from the trpc rxjs implementation to actual rxjs.