Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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

Open
gfortaine wants to merge 4 commits into Azure:main
base: main
Choose a base branch
Loading
from gfortaine:main

Conversation

Copy link

@gfortaine gfortaine commented Jan 2, 2023
edited
Loading

  • 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

blockbond, jordn, BuddhaBing, iansinnott, transitive-bullshit, blerrgh, julien-c, laygir, hperrin, isnolan, and 15 more reacted with thumbs up emoji
Copy link
Author

transitive-bullshit reacted with eyes emoji

@gfortaine gfortaine force-pushed the main branch 2 times, most recently from 344365b to b8f0a4e Compare January 2, 2023 20:45
Copy link
Author

Copy link

lexich commented Jan 11, 2023

Hey, @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?

luckydrq reacted with thumbs up emoji gfortaine reacted with heart emoji

Copy link
Author

gfortaine commented Jan 11, 2023
edited
Loading

lexich reacted with heart emoji

Copy link

jordn commented Jan 12, 2023

Thank you very much @gfortaine! Just what I was hoping for.

gfortaine reacted with heart emoji

Copy link
Author

@firedog1024 Wouldn't Microsoft mind to prioritize this PR, please ?

laygir, joshmossas, and griga reacted with eyes emoji

Copy link

waylaidwanderer commented Feb 8, 2023
edited
Loading

@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, laygir, and hperrin reacted with thumbs up emoji

Copy link

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

@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.

jspahrsummers, blerrgh, isnolan, and ShafSpecs reacted with thumbs up emoji

Copy link

julien-c commented Apr 5, 2023

would be awesome to get this merged, IMO

gfortaine, laygir, trevorpfiz, David-Kristek, srolel, Tomas2D, Pierrci, gsaivinay, and griga reacted with thumbs up emoji

K-Kielak added a commit to quantum-boost/fetch-event-source that referenced this pull request Apr 11, 2023
Copy link

hperrin commented Jun 13, 2023

@waylaidwanderer Your fix works a treat!

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

Copy link

@vishwam can we get this merged please?

Copy link

@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)

Copy link

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

Copy link

@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

Copy link

@vishwam @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

spersico reacted with thumbs up emoji spersico, roxi3906, and Congee reacted with eyes emoji

Copy link

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

Copy link

What's the status of this PR?

Copy link

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 🙏

Copy link

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

Copy link

You would see it happening here.

andresgutgon reacted with thumbs up emoji

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

Reviewers

1 more reviewer

@BuddhaBing BuddhaBing BuddhaBing left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /