search

dan-cooke / remix-sse

Server Sent Events (SSE) for the remix framework
Apache License 2.0
78 stars 3 forks source link

I believe `addNewEvent` doesn't work as expected #26

Open wallpants opened 6 months ago

wallpants commented 6 months ago

https://github.com/dan-cooke/remix-sse/blob/6eaccce1295514c7b55f6c4176fe35a5f4d40fac/packages/client/src/addNewEvent.ts#L3

hey there, thanks for the cool package. I'm reading though the source code to learn a bit about how you built this and my strict typescript/eslint config is raising a bunch of warnings, some of which are stylistic but some others I think not so much.

One that caught my eye was this line of code.

It's my understanding that [].shift() returns the removed element, not the modified array. From what I understand that would mean that you're actually spreading the removed element rather than concatenating arrays. Haven't tested it myself, but I'd be surprised if the addNewEvent function worked the way I believe you expect it to.

wallpants commented 6 months ago

I think you can fix that with something like this:

export const addNewEvent = (event: any, events: any[], maxEvents: number) => {
  while (events.length > maxEvents) events.shift();
  return [...events, event];
};

You could replace the if for a while just to make sure that if you ever got to a state where for some reason you have more than just 1 extra element in the array that allowed, the array will be truncated as needed.

dan-cooke commented 6 months ago

You are right. I must have missed this - I still need to add a proper test suite.

PR would be very much welcome! :) otherwise I can get round to this fix soon

dan-cooke commented 6 months ago

@wallpants actually, this should be working, which is even more confusing to me.

If you look at these lines in useSubscribe

You can see I am always passing a maxEvents so therefore the library would break at events over 50.

Which is definetly not the case.

I will need to investigate this further though. Thanks for the report!