Skip to content
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

eventsource@v3: looking for testers #329

Open
rexxars opened this issue Nov 14, 2024 · 4 comments
Open

eventsource@v3: looking for testers #329

rexxars opened this issue Nov 14, 2024 · 4 comments

Comments

@rexxars
Copy link
Member

rexxars commented Nov 14, 2024

eventsource v3

TL;DR

I want to modernize this module, making it both easier to maintain and more cross-platform friendly.

You can help test the next version of this module by installing eventsource@next from npm, and reading the migration guide to see how to update your code.

Any issues you find can be reported on this issue. The code for the new version is available in the v3 branch.

Background

This package is soon to be 13 years old. When Aslak and Einar created the first version of this module, node.js wasn't even at 0.10 yet. A lot has changed since those days:

  • Node.js, browsers and other Javascript environments have aligned more closely. We now have the fetch API available in all modern browsers, Node.js, Deno, Bun etc. A recent addition is the "web streams" API - also available in the same environments.
  • The module system discussion has changed. While CommonJS is still used, more and more tooling and modules are moving towards ES Modules.
  • TypeScript has become the defacto standard for writing type-safe JavaScript. This module was written in the days before TypeScript was a thing, and currently relies on an external @types module for TypeScript support.
  • With the advent of AI/LLMs, EventSource/Server-Sent Events are becoming more relevant - many providers use it as the protocol to stream responses to clients.

Proposal

Use ES Modules (ESM), with CommonJS fallback

We'll build the module using @sanity/pkg-utils (because we use it where I work, and I am familiar with it), and use conditional exports to provide both ESM and CommonJS variants. Will also switch to named exports, since it has a better cross-module system compatibility.

Split the parser and the client

I've made a parser module for EventSource that is minimal, well tested and works in "any" JavaScript environment. Splitting the concerns makes sense from a testing and maintenance perspective.

Use TypeScript for type safety

I find it easier to maintain and think about code when it's typed. Not having to keep the @types/eventsource module in sync with the main module is a big plus.

Use fetch, ReadableStream, EventTarget

Allows us to reuse the same code across Node.js, browsers and other environments that support the fetch API. The polyfill for browsers becomes very slim, and there are few behavioral differences between the environments.

Remove (most) request-based extensions

In v2, there are some extensions to the WhatWG spec - allowing users to pass custom headers, adding support for proxies, specifying HTTPS options etc. I propose that all of these are removed, and instead left up to the user to implement: they are trivially implemented by passing a custom fetch function. This creates a single extension point, making the module easier to maintain.

Drop support for Node.js < 18

In order to use fetch, web streams and other modern APIs, we need to drop support for Node.js versions < 18. This is a tradeoff I'm willing to make, as Node 18 is going out of LTS in April 2024 and older versions are already out of LTS. Users who are on older versions can still rely on v2 of this module.

Repo: Use conventional commits, automate releases

In order to make the module easier to maintain, I propose we switch to using conventional commits, and automate releases using GitHub Actions. This will make it easier to maintain the module, and make it easier for contributors to understand the release process. Automatic generation of changelogs is a nice bonus.

Repo: Move to main branch

I propose we move the default branch from master to main. This better aligns with the new default branch naming in GitHub.

Disclaimer

While these changes may seem drastic, they are necessary for me to continue supporting this module long term. It also falls within the bounds of what Aslak said when he passed on the maintainership of the module to me:

Be conservative - if you allow non-standard extensions to the W3C API, only do so if it cannot be done easily with a wrapper around the library.

This is exactly what I am proposing, by encapsulating all/most non-standard extensions in a single extension point (the fetch property).

Dependency free

While not 100% true, it only has a single dependency - eventsource-parser, which has zero dependencies of its own.

Keep it working in browsers

This is probably the biggest change - in a positive way. Instead of having to rely on a webpacked file that sets some browser globals, we can now use the module directly in the browser, and with no node.js-polyfills bundled.

Keep it open

I will continue to welcome contributions, but also want to ensure that we keep an eye on security. 19+ million downloads per month means we have a responsibility to our users. By making releases built and signed by GitHub actions, the risk of malicious code being published by someone accidently gaining access to an npm account is reduced.

Preliminary test results

I've tried to exercise the new module by running a test suite that connects to a Server-Sent Events endpoint which emits events every ~10 seconds, sends comments as "keep-alives" (technically not needed given the 10s event interval) and occasionally closes the connection due to the load balancer in front of the endpoint terminating long-lived connections. The tests are run on Node.js, bun and in browsers. In server environments, I am testing the old EventSource@2 implementation vs the new one, and in browsers I am running the new implementation vs the browser native implementation and vs the yaffle polyfill. I am not running it against the browser-compiled variant of EventSource@2 since I consider it almost "an accident" that it works in browsers, given the amont of polyfills for node internals it uses.

This is all running on my laptop on a wifi connection which tends to be online, but sometimes goes into slumber or drops the wifi or similar. The benefit here is that I get to test and compare both the "optimal" case as well as the "difficult" cases (wifi disconnect, computer goes into sleep mode) and see that things recover as expected.

The results are promising:

  • The old implementation actually seems to have problems with stalling at times, in Node.js. It simply stops receiving events. The new implementation doesn't stall - it seems to be much better at recovering.
  • The browser implementation works flawlessly and has a 1:1 match with the browser native API.

One thing I did discover with the browser variant of the new module is that the Last-Event-ID header is not a CORS safelisted header, and thus reconnections cross-origin causes a preflight request to check if allowed to send the header. It seems odd that the native EventSource is allowed to send this header but not a fetch request, and I'm now working on rectifying this in the WhatWG fetch spec. Until then, we have two options:

  • Document the CORS behavior (my preference) and tell people how to respond to an OPTIONS request
  • Change browsers to use a query string parameter for the last event ID. This is what the yaffle polyfill does (lastEventId=<id>), as well as the amvtek polyfill (evs_last_event_id=<id>). Since this isn't standardized, I am not super eager to implement this - it could also be done manually by passing a custom fetch implementation which replaces the last-event-id header with a query string parameter of choice.
@i18nsite
Copy link

i18nsite commented Nov 15, 2024

I use eventsource3, and the program will exit inexplicably after running for a few hours without any alarm (I open and close a different eventsource every minute). With eventsource2, there is no such problem.

my code is like below

gen = (prompt, style, width, height) =>
  seed = Math.floor( Math.random() * 2147483647 )
  session_hash = Math.floor(Math.random()*1e13).toString(36)

  bodyData = {
    data: [
      prompt, # 正面词
      "", # 负面词
      style, # 风格
      false,
      1,
      seed,
      height,
      width,
      1,
      1,
      STEP,
      true
    ]
    event_data: null
    fn_index: 4
    trigger_id: 8
    session_hash
  }

  r = await fetch API+"queue/join?",
    headers:
      accept: "*/*"
      "accept-language": "zh-CN,zh;q=0.9,zh-TW;q=0.8,zh-HK;q=0.7,en;q=0.6"
      "content-type": "application/json"
      priority: "u=1, i"
      "sec-ch-ua": "\"Google Chrome\";v=\"131\", \"Chromium\";v=\"131\", \"Not_A Brand\";v=\"24\""
      "sec-ch-ua-mobile": "?0"
      "sec-ch-ua-platform": "\"macOS\""
      "sec-fetch-dest": "empty"
      "sec-fetch-mode": "cors"
      "sec-fetch-site": "same-origin"
    referrer: "https://nv-sana.mit.edu/"
    referrerPolicy: "strict-origin"
    body: JSON.stringify bodyData
    method: "POST"
    mode: "cors"
    credentials: "omit"

  console.log await r.text()

  esurl = API+"queue/data?session_hash="+session_hash
  es = new EventSource(esurl)
  espromise = new Promise (resolve, reject) =>
    es.onmessage = ({data}) =>
      data = JSON.parse data
      {msg} = data
      switch msg
        when 'estimation'
          console.log '预期', Math.round(data.rank_eta), '秒'
        when 'progress'
          for i from data.progress_data
            console.log i.desc
        when 'process_completed'
          for {image} from data.output.data[0]
            {url} = image
            hash = Buffer.from(
              new Uint8Array await crypto.subtle.digest(
                'SHA-256'
                utf8e url
              )
            ).toString('base64url')
            if style == '(No style)'
              style = 'default'
            fname = style+'.'+hash+'.jpg'
            console.log fname
            await wget url, join(JPG, fname)
            es.onerror = es.onclose = =>
            resolve()
            es.close()
        when 'unexpected_error'
          console.log msg
          reject data.message
        else
          if ! ['close_stream', 'heartbeat', 'process_starts'].includes msg
            console.log '>',data
      return
    es.onerror = (err)=>
      if err?.message
        reject err.message
        # if not (err.defaultPrevented == false and err.cancelable == false)
        #   console.log 'EventSource error:', err
        #   reject err
      return
    es.onclose = =>
      resolve()
      return
    return
  # await r.text() # 必须等一下

  try
    return await espromise
  finally
    es.close()
  return

WH_LI_LEN = WH_LI.length

do =>
  {prompt:prompt_li}= Yml ROOT
  loop
    for [en,zh] from prompt_li
      [w,h] = randWh()
      console.log '\n\n'+zh+'\n\n'+en+'\n'
      loop
        try
          await gen(en, randStyle(), w, h)
          break
        catch e
          console.error 'gen error', e
  console.log 'exit'
  return

@rexxars
Copy link
Member Author

rexxars commented Nov 16, 2024

I use eventsource3, and the program will exit inexplicably after running for a few hours without any alarm (I open and close a different eventsource every minute). With eventsource2, there is no such problem.

Under which environment is this? Node.js? Which version? I'm not familiar with the language above, but could you add a catch handler to your promise that awaits the espromise that logs any errors?

@i18nsite
Copy link

i think
await wget url, join(JPG, fname)
throw a error in onmessage

event2 will log the error
3 not log the error

@rexxars
Copy link
Member Author

rexxars commented Nov 16, 2024

i think await wget url, join(JPG, fname) throw a error in onmessage

event2 will log the error 3 not log the error

I am unable to reproduce. I've tried recreating something that follows your code here: https://github.com/rexxars/es-repro-329

You can switch between EventSource@v2 and EventSource@next by swapping the EventSource variable. When running this, I get (in both v2 and v3):

> [email protected] start
> node repro.js

/Users/espenh/webdev/es-repro/repro.js:64
    setTimeout(reject, 100, new Error('wget error'))
                            ^

Error: wget error
    at /Users/espenh/webdev/es-repro/repro.js:64:29
    at new Promise (<anonymous>)
    at wget (/Users/espenh/webdev/es-repro/repro.js:63:10)
    at es.onmessage (/Users/espenh/webdev/es-repro/repro.js:24:13)
    at /Users/espenh/webdev/es-repro/node_modules/eventsource-v3/dist/index.cjs:55:119
    at dispatchEvent (/Users/espenh/webdev/es-repro/node_modules/eventsource-parser/dist/index.cjs:69:24)
    at parseLine (/Users/espenh/webdev/es-repro/node_modules/eventsource-parser/dist/index.cjs:22:7)
    at Object.feed (/Users/espenh/webdev/es-repro/node_modules/eventsource-parser/dist/index.cjs:17:7)
    at /Users/espenh/webdev/es-repro/node_modules/eventsource-v3/dist/index.cjs:44:46
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

Are you able to modify my test repo code to recreate the case you are seeing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants