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

[DRAFT] feat(ext/fetch): Implement EventSource #12350

Closed
wants to merge 29 commits into from
Closed

[DRAFT] feat(ext/fetch): Implement EventSource #12350

wants to merge 29 commits into from

Conversation

MierenManz
Copy link

@MierenManz MierenManz commented Oct 7, 2021

closes #10298
This still needs some work.

  • update WPT expectations
  • Use Primordials
  • Use Ops instead of fetch
  • Use Symbols
  • Typings
  • Add to 99_main.js

@CLAassistant
Copy link

CLAassistant commented Oct 7, 2021

CLA assistant check
All committers have signed the CLA.

@MierenManz
Copy link
Author

MierenManz commented Oct 7, 2021

if you have any constructive feedback. Feel free to comment 😉

Copy link
Member

@crowlKats crowlKats left a comment

Choose a reason for hiding this comment

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

Typings are missings, loading in 99_main missing, don't use private properties but symbols.
Also dont see any need for the webidl dict being a variable and then assigned to the converter, could just inline that.

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

Some comments:

  • This needs to be changed to use primordials. No globals should be used.
  • This should not depend on the fetch function. Instead op_fetch* ops should be used directly, or the event source parsing should be moved to Rust (i.e. op_eventsource + op_eventsource_next).

@MierenManz
Copy link
Author

I think I'll go the op fetch route. I don't really see the need to do this stuff in rust.

ext/fetch/27_eventsource.js Outdated Show resolved Hide resolved
ext/fetch/27_eventsource.js Outdated Show resolved Hide resolved
ext/fetch/27_eventsource.js Show resolved Hide resolved
@stale
Copy link

stale bot commented Dec 14, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Dec 14, 2021
@stale stale bot closed this Dec 21, 2021
@crowlKats crowlKats reopened this Feb 12, 2022
@stale stale bot removed the stale label Feb 12, 2022
@MierenManz
Copy link
Author

This is quite a messy PR. So I've talked with @crowlKats and decided that I'm gonna implement this from scratch rather than trying to fix this PR. I will open that hopefully today or tomorrow

@MierenManz MierenManz closed this Feb 14, 2022
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.

Create a EventSource implementation
5 participants