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

Don't use browser Request api, make our own simple one #106

Closed
JoelEinbinder opened this issue Jul 24, 2017 · 3 comments
Closed

Don't use browser Request api, make our own simple one #106

JoelEinbinder opened this issue Jul 24, 2017 · 3 comments
Assignees

Comments

@JoelEinbinder
Copy link
Collaborator

JoelEinbinder commented Jul 24, 2017

@aslushnikov can you fill in why? I just have it in my notes that we should do it.

@JoelEinbinder JoelEinbinder modified the milestone: v0.1 Jul 25, 2017
@Garbee
Copy link
Contributor

Garbee commented Jul 25, 2017

What exactly would this kind of API look like? I think not using the built in browser systems where available is harmful. As then developers need to learn yet-another-syntax to get the same results. Even though we already have (and are probably using) exactly what they know underneath.

@aslushnikov
Copy link
Contributor

What exactly would this kind of API look like?

We'll get rid of Headers object and use something like a map instead. It would be consistent with page.setHTTPHeaders methods.

I think not using the built in browser systems where available is harmful. Even though we already have (and are probably using) exactly what they know underneath.

The API in question is consumed by node.js, not by browser. We don't have fetch api in node. We have protocol's network domain which we expose via the puppeteer's request and puppeteer's response classes.

Browser also has Request and Response classes, so we tried to align puppeteer's request and response with the browser counterparts. It turned out that:

Since we failed to implement browser api to a reasonable extend, we don't want to commit to it any more.

@Garbee
Copy link
Contributor

Garbee commented Jul 25, 2017

Ah, thanks for the clarification @aslushnikov. Knowing that it SGTM to make this change.

@aslushnikov aslushnikov mentioned this issue Jul 27, 2017
aslushnikov added a commit that referenced this issue Jul 27, 2017
This patch removes Header class and substitutes it with a simple
Map object.

The map is chosen over the vanilla object since it has explicit
order of headers which we'd like to preserve.

References #106.
aslushnikov added a commit that referenced this issue Jul 27, 2017
This patch:
- removes Body.arrayBuffer. This method is redundant since there's
  already a Body.buffer() method
- removes Body.bodyUsed getter.

References #106
@aslushnikov aslushnikov mentioned this issue Aug 17, 2017
3 tasks
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

No branches or pull requests

3 participants