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.

Sign up for GitHub

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

Jump to bottom

lib: add EventSource #51575

Merged
merged 1 commit into from May 13, 2024
Merged

lib: add EventSource #51575

merged 1 commit into from May 13, 2024

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Jan 26, 2024 •
edited

Proposal for Release Notes:

EventSource

EventSource is a W3C compliant Client for Server-sent events. The new flag --experimental-eventsource can be used to expose a global EventSource-constructor.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/security-wg

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jan 26, 2024
@Uzlopak Uzlopak mentioned this pull request Jan 26, 2024
Merged
7 tasks
@benjamingr
Copy link
Member

(LGTM is in principle, this is a draft)

@jeluard jeluard mentioned this pull request Mar 13, 2024
Open
@KhafraDev
Copy link
Member

@Uzlopak this can move forward now

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Apr 14, 2024

Roger that.

@Uzlopak Uzlopak force-pushed the implement-eventsource-undici branch from 6a3a146 to 76fbb0a Compare April 14, 2024 22:10
@Uzlopak Uzlopak marked this pull request as ready for review April 14, 2024 22:10
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Apr 14, 2024

@KhafraDev

PTAL

@Uzlopak Uzlopak force-pushed the implement-eventsource-undici branch from 76fbb0a to bccbba6 Compare April 14, 2024 22:15
@Uzlopak Uzlopak changed the title feat: implement eventsource undici lib: add EventSource Apr 14, 2024
@Uzlopak Uzlopak force-pushed the implement-eventsource-undici branch 2 times, most recently from 6a77c9c to c06cd4c Compare April 14, 2024 22:32
@KhafraDev
Copy link
Member

cc @nodejs/web-standards

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Apr 15, 2024

Can this be a notable change ;)?

@marco-ippolito marco-ippolito added the notable-change PRs with changes that should be highlighted in changelogs. label Apr 15, 2024
@github-actions GitHub Actions
Copy link
Contributor

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @marco-ippolito.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 15, 2024
mcollina
mcollina approved these changes Apr 15, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 15, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/58390/

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Apr 16, 2024

Can somebody please add author-ready flag?

@RafaelGSS
Is the description ok?

@marco-ippolito
Copy link
Member

marco-ippolito commented Apr 16, 2024 •
edited

Can somebody please add author-ready flag?

@RafaelGSS
Is the description ok?

Can you expand a bit more?
For author ready CI must be green

@mcollina mcollina added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 16, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/58422/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/58435/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/58783/

@mcollina
Copy link
Member

@Uzlopak there are conflicts snow

@Uzlopak Uzlopak force-pushed the implement-eventsource-undici branch from c06cd4c to 2eb88a8 Compare May 5, 2024 11:31
@Uzlopak
Copy link
Contributor Author

Uzlopak commented May 5, 2024

@mcollina

PTAL

@Uzlopak Uzlopak force-pushed the implement-eventsource-undici branch from 2eb88a8 to 07f9ccb Compare May 5, 2024 11:57
mcollina
mcollina approved these changes May 5, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label May 5, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 5, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/58960/

@aduh95
Copy link
Contributor

aduh95 commented May 11, 2024

For author ready CI must be green

@marco-ippolito running CI is enough to be author ready

node/doc/contributing/collaborator-guide.md

Lines 895 to 899 in 956521c

* `author-ready` - A pull request is _author ready_ when:
* There is a CI run in progress or completed.
* There is at least one collaborator approval (or two TSC approvals for
semver-major pull requests).
* There are no outstanding review comments.

aduh95
aduh95 reviewed May 11, 2024
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add an entry in the ESLint config to forbid the use of the global in the lib/ folder please? Similar to

node/lib/.eslintrc.yaml

Lines 157 to 158 in 3136fb0

- name: fetch
message: Use `const { fetch } = require('internal/deps/undici/undici');` instead of the global.

@marco-ippolito
Copy link
Member

For author ready CI must be green

@marco-ippolito running CI is enough to be author ready

node/doc/contributing/collaborator-guide.md

Lines 895 to 899 in 956521c

* `author-ready` - A pull request is _author ready_ when:
* There is a CI run in progress or completed.
* There is at least one collaborator approval (or two TSC approvals for
semver-major pull requests).
* There are no outstanding review comments.

Thanks for clarifying, I was sure it required green ci 😵

@Uzlopak Uzlopak force-pushed the implement-eventsource-undici branch from 07f9ccb to 8165053 Compare May 11, 2024 23:00
@Uzlopak
Copy link
Contributor Author

Uzlopak commented May 11, 2024

@aduh95

done ;)

aduh95
aduh95 approved these changes May 11, 2024
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it surprising that there are no WPT changes, but that shouldn't block this from landing.

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels May 11, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 11, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59133/

@KhafraDev
Copy link
Member

I find it surprising that there are no WPT changes, but that shouldn't block this from landing.

wdym? we run WPTs for it in undici

@aduh95
Copy link
Contributor

aduh95 commented May 12, 2024

wdym? we run WPTs for it in undici

I would expect that we run WPT tests for it in this repo.

@mcollina
Copy link
Member

wdym? we run WPTs for it in undici

I would expect that we run WPT tests for it in this repo.

We don't run wpts for fetch either on this as far as I know.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59180/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59194/

@marco-ippolito marco-ippolito added the commit-queue Add this label to land a pull request using GitHub Actions. label May 13, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 13, 2024
@nodejs-github-bot nodejs-github-bot merged commit 1d7d094 into nodejs:main May 13, 2024
59 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 1d7d094

@Uzlopak Uzlopak deleted the implement-eventsource-undici branch May 13, 2024 05:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@benjamingr benjamingr benjamingr approved these changes

@aduh95 aduh95 aduh95 approved these changes

@KhafraDev KhafraDev KhafraDev approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Uzlopak @nodejs-github-bot @benjamingr @KhafraDev @marco-ippolito @mcollina @aduh95