Skip to content

V3 Upgrade : ESM, TypeScript 4.9 & Node.js support - #28

Open
Guillaume FORTAINE (gfortaine) wants to merge 4 commits into
Azure:mainfrom
gfortaine:main
Open

V3 Upgrade : ESM, TypeScript 4.9 & Node.js support#28
Guillaume FORTAINE (gfortaine) wants to merge 4 commits into
Azure:mainfrom
gfortaine:main

Conversation

@gfortaine

@gfortaine Guillaume FORTAINE (gfortaine) commented Jan 2, 2023

Copy link
Copy Markdown
  • feat(upgrade): migrate to ESM
  • feat(upgrade): add exports
  • feat(upgrade): upgrade to TypeScript 4.9
  • feat(upgrade): add node.js support
  • feat(upgrade): adjust parse.getMessages signature

@gfortaine

Copy link
Copy Markdown
Author

cc Vishwam Subramanyam (@vishwam)

@gfortaine

Copy link
Copy Markdown
Author

Here is the package 🎉 : https://www.npmjs.com/package/@fortaine/fetch-event-source

@lexich

Copy link
Copy Markdown

Hey, Guillaume FORTAINE (@gfortaine), you've done a perfect job. Is it strictly required to use "node": ">=18.12" I want to use your package while official is in progress, but I use node16. Can you downgrade this requirement?

@gfortaine

Guillaume FORTAINE (gfortaine) commented Jan 11, 2023

Copy link
Copy Markdown
Author

Alexey Efremov (@lexich) Done (Node v16.15.0 (LTS) w/ experimental support to the fetch API) ✅ : https://www.npmjs.com/package/@fortaine/fetch-event-source/v/3.0.5?activeTab=explore

@jordn

Copy link
Copy Markdown

Thank you very much Guillaume FORTAINE (@gfortaine)! Just what I was hoping for.

Comment thread src/fetch.ts Outdated
Comment thread src/fetch.ts Outdated
@gfortaine

Copy link
Copy Markdown
Author

Ian Hollier (@firedog1024) Wouldn't Microsoft mind to prioritize this PR, please ?

@waylaidwanderer

Joel (waylaidwanderer) commented Feb 8, 2023

Copy link
Copy Markdown

Guillaume FORTAINE (@gfortaine) On Node 16.15.1, with a node-fetch polyfill, it gives the following error:

TypeError: stream.getReader is not a function                                                                                                                                                                                                                                         
    at getBytes (file:///.../node_modules/@fortaine/fetch-event-source/lib/esm/parse.js:2:27)
    at create (file:///.../node_modules/@fortaine/fetch-event-source/lib/esm/fetch.js:54:23)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

Edit: replacing getBytes with this seems to fix it:

export async function getBytes(body, onChunk) {
    for await (const chunk of body) {
        onChunk(chunk);
    }
}

But a check should be made to see if getReader is a function first and use the appropriate version of the code.

@transitive-bullshit

Copy link
Copy Markdown

Here's some additional Node.js-specific cases to be aware of: https://github.com/transitive-bullshit/chatgpt-api/blob/main/src/fetch-sse.ts#L27-L48

Guillaume FORTAINE (@gfortaine) you may want to just publish your fork's source as your package's source. Microsoft projects aren't always the best at incorporating community PRs.

@julien-c

Copy link
Copy Markdown

would be awesome to get this merged, IMO

Kacper Kielak (K-Kielak) added a commit to quantum-boost/fetch-event-source that referenced this pull request Apr 11, 2023
@hperrin

Copy link
Copy Markdown

Joel (@waylaidwanderer) Your fix works a treat!

I added that fix to Guillaume FORTAINE (@gfortaine)'s fixes and published them here: https://www.npmjs.com/package/fetch-event-source-hperrin

@BuddhaBing

Copy link
Copy Markdown

Vishwam Subramanyam (@vishwam) can we get this merged please?

@BuddhaBing

Copy link
Copy Markdown

Guillaume FORTAINE (@gfortaine) can you add this fix to your PR please (and ideally your package also, since I'm currently using that until this PR is merged)

@eugeneYWang

Copy link
Copy Markdown

If this can be be fixed and released, many people, including me, would be helped !

@eugeneYWang

Copy link
Copy Markdown

Guillaume FORTAINE (@gfortaine) can you add this fix to your PR please (and ideally your package also, since I'm currently using that until this PR is merged)

Fontaine's lib would work well under NodeJS 18.17.1. Even without the fix that you referred to. Guess Native Fetch implementation has been improved, and connection header seems to be not forbidden as well

@BuddhaBing

Copy link
Copy Markdown

Vishwam Subramanyam (@vishwam) Ian Hollier (@firedog1024) has this project been abandoned? What's the issue with getting this merged? Clearly a lot of people are eager for it, given the comments here

@dargmuesli

Copy link
Copy Markdown

You might want to check out extended-eventsource as potential new replacement. It's codebase looks proper.

@andresgutgon

Copy link
Copy Markdown

What's the status of this PR?

@dargmuesli

Copy link
Copy Markdown

Please don't post redundant comments, it pings everyone interested in actually resolving this issue without adding much value. Let's spend that time on PRs for maintained projects 🙏

@andresgutgon

Copy link
Copy Markdown

Ok but what would be the right way to know if this is moving forward? Sorry for the noise

@dargmuesli

Copy link
Copy Markdown

You would see it happening here.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.