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

refactor: use primordials in extensions/fetch #11266

Merged
merged 7 commits into from
Jul 6, 2021

Conversation

satyarohith
Copy link
Member

@satyarohith satyarohith commented Jul 4, 2021

  • extensions/fetch/01_fetch_util.js
  • extensions/fetch/20_headers.js
  • extensions/fetch/21_formdata.js
  • extensions/fetch/22_body.js
  • extensions/fetch/22_http_client.js (refactor not required)
  • extensions/fetch/23_request.js
  • extensions/fetch/23_response.js
  • extensions/fetch/26_fetch.js

Best reviewable commit by commit.

Towards #11224

@satyarohith satyarohith mentioned this pull request Jul 4, 2021
67 tasks
@satyarohith satyarohith changed the title refactor: use primordials in ext/fetch refactor: use primordials in extensions/fetch Jul 4, 2021
Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

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

It's fun reviewing these :-)

extensions/fetch/20_headers.js Show resolved Hide resolved
extensions/fetch/20_headers.js Show resolved Hide resolved
extensions/fetch/20_headers.js Show resolved Hide resolved
extensions/fetch/20_headers.js Show resolved Hide resolved
extensions/fetch/20_headers.js Show resolved Hide resolved
extensions/fetch/20_headers.js Show resolved Hide resolved
extensions/fetch/20_headers.js Show resolved Hide resolved
extensions/fetch/22_body.js Show resolved Hide resolved
extensions/fetch/23_request.js Show resolved Hide resolved
extensions/fetch/23_response.js Show resolved Hide resolved
@SimonRask
Copy link
Contributor

@satyarohith 01_fetch_util.js needs TypeError added :)

Are you not done since it's a draft?

@satyarohith satyarohith marked this pull request as ready for review July 5, 2021 19:17
Copy link
Contributor

@SimonRask SimonRask left a comment

Choose a reason for hiding this comment

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

20_headers.js:

  • line 299 ArrayPrototypeSplice
  • line 403 SymbolFor
  • line 411 SymbolToStringTag
  • line 423 SymbolIterator

21_formdata.js:

  • line 70 SymbolToStringTag

22_body.js:

  • line 300 ArrayPrototypeMap
  • line 340 ArrayPrototypeSlice

23_request.js:

  • line 387 SymbolToStringTag
  • line 391 SymbolFor

23_response.js:

  • line 370 SymbolToStringTag
  • line 374 SymbolFor

26_fetch.js

  • line 210 PromisePrototypeCatch
  • line 220 PromisePrototypeCatch
  • line 240 PromisePrototypeCatch
  • line 309 ArrayPrototypeFilter
  • line 363 ArrayPrototypePush
  • line 407 ArrayPrototypePush
  • line 411 PromisePrototypeThen
  • line 432 PromisePrototypeCatch
  • line 475 StringPrototypeToLowerCase

extensions/fetch/21_formdata.js Show resolved Hide resolved
extensions/fetch/21_formdata.js Outdated Show resolved Hide resolved
extensions/fetch/23_request.js Show resolved Hide resolved
extensions/fetch/23_request.js Outdated Show resolved Hide resolved
extensions/fetch/23_response.js Show resolved Hide resolved
extensions/fetch/26_fetch.js Outdated Show resolved Hide resolved
@satyarohith
Copy link
Member Author

satyarohith commented Jul 6, 2021

Thanks for the thorough review, @SimonRask & @littledivy! :)

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM

@bartlomieju bartlomieju merged commit ab6b0ce into denoland:main Jul 6, 2021
@satyarohith satyarohith deleted the refactor_fetch branch July 6, 2021 09:39
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.

None yet

4 participants