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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants