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 in sync with node-fetch #43

Closed

Conversation

tinovyatkin
Copy link
Member

@tinovyatkin tinovyatkin commented Jun 7, 2020

This may be too much for one PR, but on other hand, this thing is small, so, it easier to review all in one.

This PR sync fetch-blob with latest changes in node-fetch@3:

  1. Bumps minimum Node version to 10.17 and uses async functions and Readable.from
  2. Adds JSDoc descriptions from MDN for all functions/properties
  3. Drop manually created TypeScript typing and generate one using TypeScript from JSDoc at prepublishOnly - as resulted the reference to DOM library is removed (as discussed in node-fetch). Run npm run prepublishOnly to see the output.
  4. Simplifies .slice method by using Buffer.subarray
  5. Some small refactorings on the way.

No changes to tests were made.

@codecov
Copy link

codecov bot commented Jun 7, 2020

Codecov Report

Merging #43 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #43   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           62        39   -23     
  Branches        12         8    -4     
=========================================
- Hits            62        39   -23     
Impacted Files Coverage Δ
index.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 870be1e...a320b45. Read the comment docs.

@Richienb Richienb changed the title refactor in sync with node-fetch Refactor in sync with node-fetch Jun 7, 2020
@jimmywarting
Copy link
Contributor

I have another PR that will make this more obsolete on the way to resolve #40

@jimmywarting
Copy link
Contributor

jimmywarting commented Jun 7, 2020

did you know that Buffer.slice differ from Uint8Array.slice and behaves like Buffer.subarray?

Returns a new Buffer that references the same memory as the original, but offset and cropped by the start and end indices.

This is the same behavior as buf.subarray().

This method is not compatible with the Uint8Array.prototype.slice(), which is a superclass of Buffer. To copy the slice, use Uint8Array.prototype.slice().

https://nodejs.org/api/buffer.html#buffer_buf_slice_start_end

But i think node should never have added that, i do agree that subarray is better, if buffer extends Uint8Array and you can treat it as an regular Uint8Array to be isomorphic and uses either Buffer or Uint8Array on browser or node then it should behave the same way i think.

@tinovyatkin
Copy link
Member Author

tinovyatkin commented Jun 7, 2020

I have another PR that will make this more obsolete on the way to resolve #40

I've basically tried to improve the file fitness of the class, removing the obsolete code, and making things easier. Do you prefer to wait with this PR until your changes merged and then roll it over, or should we merge this first?

@jimmywarting
Copy link
Contributor

I have also bumped the node v to 10.17, used readable.from (also copied your jsDoc)

will put up the PR now

@jimmywarting
Copy link
Contributor

Sorry for handing you some conflicts

@tinovyatkin tinovyatkin closed this Jun 8, 2020
@tinovyatkin tinovyatkin deleted the finish-class-refactoring branch June 8, 2020 01:54
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.

2 participants