Skip to content

Conversation

LinusU
Copy link
Member

@LinusU LinusU commented May 20, 2020

Alternative approach to #37

I think that the reason that these were included in the first place was because they weren't present in upstream TypeScript at the time. This have been fixed now with: microsoft/TypeScript#36164

The drawback of this is that we are removing [Symbol.toStringTag] and toString. But if we think that it's correct to have them on the Blob I think it would be better to submit patches upstream to add that instead, since the idea behind this package is to bring Blob cross platform.

Since we haven't yet published a version with typings this would not be a breaking change!

Closes: #37

@codecov
Copy link

codecov bot commented May 20, 2020

Codecov Report

Merging #38 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #38   +/-   ##
=======================================
  Coverage   86.95%   86.95%           
=======================================
  Files           1        1           
  Lines          69       69           
  Branches       13       13           
=======================================
  Hits           60       60           
  Misses          9        9           

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 6fabfce...d87f596. Read the comment docs.

@LinusU LinusU mentioned this pull request May 20, 2020
@LinusU
Copy link
Member Author

LinusU commented May 20, 2020

Tests are failing because xo doesn't realise that Blob is defined (funny it worked with extends 😄)

edit: fixed, amended commit, and pushed

@LinusU
Copy link
Member Author

LinusU commented May 21, 2020

Thanks for the quick reviews ❤️

@LinusU LinusU merged commit bec5a3d into master May 21, 2020
@delete-merged-branch delete-merged-branch bot deleted the blob-typings branch May 21, 2020 08:12
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants