razshare / sveltekit-sse

Server Sent Events with SvelteKit
https://www.npmjs.com/package/sveltekit-sse
MIT License
301 stars 9 forks source link

Resubscribing sends empty value #48

Closed nietsuu closed 5 months ago

nietsuu commented 5 months ago

I'm not really sure how things should work, but whenever I resubscribe after some time:

function subscribe() {
    const sse = source(...).select(...).json();
    sse.subscribe((value) => {
        console.log(value);
    });
}

subscribe();  // Prints correct value
subscribe();  // Still prints correct value
subscribe();  // As long as its called consecutively, it prints the correct value

// HOWEVER when it's called after some time
// even if it's just 100 ms
setTimeout(() => {
    subscribe();  // Prints `undefined` SOMETIMES

    // I call source() 3 times (different eventName) on my actual code
    // 2 of them is undefined, and 1 is correct
    // And it's also inconsistent.
    // Sometimes, the other one is correct, sometimes it's the other
    // What I'm basically saying is that it's random.
    // Sometimes even all of them 3 are undefined
}, 100);
razshare commented 5 months ago

Hello @nietsuu , The first value of the store is always set to blank ('').

Which is probably why you're getting an undefined - the internal json parsing fails.

You can check the parsing error yourself.

<script>
  import { source } from 'sveltekit-sse';

  function subscribe() {
    const sse = source('/events')
      .select('message')
      .json(function or({ error, previous, raw }) {
        console.error({ error, raw });
        return previous;
      });
    sse.subscribe((value) => {
      console.log(value);
    });
  }

  subscribe(); // Prints correct value
  subscribe(); // Still prints correct value
  subscribe(); // As long as its called consecutively, it prints the correct value

  // HOWEVER when it's called after some time
  // even if it's just 100 ms
  setTimeout(() => {
    subscribe(); // Prints `undefined` SOMETIMES

    // I call source() 3 times (different eventName) on my actual code
    // 2 of them is undefined, and 1 is correct
    // And it's also inconsistent.
    // Sometimes, the other one is correct, sometimes it's the other
    // What I'm basically saying is that it's random.
    // Sometimes even all of them 3 are undefined
  }, 100);
</script>

image

The reason for this behavior is because the $ syntax is synchronous, while the sse stream is asynchronous, which means it takes time before you get your first valid value from the server.

There is currently no way to do this in svelte

<script>
  import { source } from 'sveltekit-sse';
  const value = source('/events').select('message').json()
</script>

{#await value then result}
    {$result}
{/await}

without changing the signature of the readable store created by source::select::json. As a matter of fact, even if I were to change the signature to Promise<Readable<...>>, you can't even use locally declared variables with the $ syntax, like $result in the example above.

You can create your own promise resolver

<script context="module">
  /**
   * @typedef MyType
   * @property {string} data
   */
</script>

<script>
  import { onMount } from 'svelte';
  import { source } from 'sveltekit-sse';

  /** @type {Promise<import('svelte/store').Readable<MyType>>} */
  const promise = new Promise(function start(resolve) {
    const store = source('/events').select('message').json();
    const unsubscribe = store.subscribe(function watch(storeValue) {
      if (storeValue) {
        unsubscribe();
        resolve(store);
      }
    });
  });

  onMount(function start() {
    /** @type {false|import('svelte/store').Unsubscriber} */
    let unsubscribe = false;
    promise.then(function run(channel) {
      unsubscribe = channel.subscribe(function watch(value) {
        console.log(value);
      });
    });
    return function stop() {
      if (unsubscribe) {
        unsubscribe();
      }
    };
  });
</script>

Then you won't get the undefined. image

I'm sure there's a better way to do that using a derived store.

That's a headache though, you're probably better of managing your default values using source::select::json.

Other than that, I can't do much going on snippets of code when it comes to specific issues like these that include timing things out.

If you can reproduce the error as you see it, I'll take another look.

I'll leave this open for now.

nietsuu commented 5 months ago

Hello @razshare! Thanks for the response.

I understand its asynchronous nature. I also know that it returns undefined at first because the store has no value yet.

My problem is different than that. I know for a fact that the store has already a value, but it still returns undefined randomly.

My initial message was lacking so I understand the misunderstanding. I'm gonna try to provide as much information now here.

console

My code basically looked like this:

subscribe();
subscribe();

setTimeout(() => {
    subscribe();
}, 1000);

Okay here's an interesting part. When I set the cache to false, I now seem to get consistent values but there still an undefined: image

razshare commented 5 months ago

Hello @nietsuu , Please provide a reproduction repository so that I can investigate. I'm not able to reproduce your case using 3 sequential subscribes and a 4th one timed out by 100 milliseconds or 1 second.

From what I'm experiencing everything works as intended. First 3 subscribes are blank. The rest have a json value.

Peek 2024-05-28 15-36

And using json() I'm getting the same result, except the value of the store is undefined instead of blank, which is intended.

Peek 2024-05-28 15-40

Without a proper reproduction I cannot help you from the looks of it.

nietsuu commented 5 months ago

Hello @razshare!

Here's the reproduction repository: https://github.com/nietsuu/test-sveltekit-sse

While I was reproducing it, I found out that the problem starts happening if I emit different eventNames.

src/routes/+server.ts:

produceSSE(["event0", "event1"]);  // undefined problem occurs
produceSSE(["event0"]);  // Works fine
razshare commented 5 months ago

Hello @nietsuu

First of all, update to version 0.13.1 to get the latest fixes described below.

I ran your reproduction and I think I figured it out. The problem isn't on your client, it's on your server.

I'll try to explain myself

First let's start from your producer on the server

image

Note that you're emitting once per event name.

Remember, connections are cached by default, meaning if you source your data AFTER that emit went off, then you're not going to get anything from the stream.

Let's omit the setTimeout for now, and go with this code

image

You'll get this output

image

which is expected - first the undefined, then the server data comes and and we get the actual json value.

Now let's introduce the setTimeout

image

You'll get

image

an extra undefined.

Which is also correct, because your stream has stopped emitting data as of 1 second ago.

Your backend producer is still locked, but it's not sending anything anymore at this point, so the readable gives your the initial default value: undefined.

[!NOTE] See note at the bottom regarding this.

Your issue can be solved in two ways - one of which was bugged as of 10 minutes, I just fixed it.

1. The oldest trick in the book - add a query string

It doesn't matter what query string, any will do, as long as its unique in your domain. image This will forcefuly open a new stream, executing your server side code again. And the result is image 3 undefiend and 3 values, which is correct.

2. The proper way - disabling caching

This was bugged, I just fixed it. image image And the result is the same as above image

I'm sending you a PR on your reproduction with the query string solution.

Let me know if this addresses your issue.

[!NOTE] There is something to be said about that initial undefined. It's unavoidable the first time a store reads the stream, the value has to be undefined. But I'm not so sure about subsequent stores that read the stream.\ I could cache the last value of the store and return that.\ I need to think about this because there's nothing in the W3C standard that suggests anything like this, and I could do more damage than good with such a change.

nietsuu commented 5 months ago

Hello @razshare,

I think I understand it now. So it actually caches just the connection. It doesn't cache the emitted value itself. That's why I get the undefined value. Is that correct?

Also, I thought all sources of the same eventName share the same store. But in actuality, every source has their own store created. That explains getting undefined on subsequent sources.

In my opinion, I think sources with the same eventName (also of the same route) should share the same store (if possible). Let me explain:

In my actual codebase, I am not actually resubscribing using setTimeout. The setTimeout was only used to simulate page navigation. So what happens in my website is, when I navigate to a different route, then I go back, that will trigger a "resubscribe". And as we already know, this will result in undefined value which is a problem.

Making the sources share a store would fix this problem. Store is a Svelte-specific feature. So even if you implement this, it wouldn't violate the W3C standard right?

Ways I know to fix my problem

Your solution: Disable caching

This works. But this also means re-executing the producer which might not be ideal on some cases. Like in my case, my producer is actually connecting to a WebSocket. I wouldn't want to keep reconnecting to the WebSocket every time I navigate to my page.

Subscribe at the root route so that it will only do it once

This works as well. But in my case, I can't do this because it wouldn't make sense to do so in my case.

Implement my own store as a wrapper to preserve the emitted values

For now, this solution is the best in my case.


Let me know if you know a better solution. Thanks for the help!

nietsuu commented 5 months ago

Also, I still don't know why this happens (caching enabled):

Case 1

produceSSE(["event0", "event1"]);  // undefined problem occurs

Case 2

produceSSE(["event0"]);  // Not undefined, but emits the last value emitted evidently from the timestamp

Case 3

produceSSE(["event1", "event0"]);  // Same as Case 2
razshare commented 5 months ago

In my opinion, I think sources with the same eventName (also of the same route) should share the same store (if possible).

Yeah this makes sense. There's not much else to say, I think you're right.

It should be easy to implement, but I can't do it right now, I'll do it in the evening and I'll let you know when it's ready.

razshare commented 5 months ago

Also, I still don't know why this happens (caching enabled):

I'll take a look into these cases too

nietsuu commented 5 months ago

It should be easy to implement, but I can't do it right now, I'll do it in the evening and I'll let you know when it's ready

Great! Thank you. Take your time, nothing urgent.

razshare commented 5 months ago

Hello @nietsuu , The fix is ready, please update to version 0.13.2 and let me know if everything works as expected on your end so that I can close this issue.

nietsuu commented 5 months ago

@razshare It works perfectly! Thank you.