-
Notifications
You must be signed in to change notification settings - Fork 29
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
fix(blob-stream): Bump major web stream polyfill v4 #116
Conversation
Codecov Report
@@ Coverage Diff @@
## main #116 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 4 4
Lines 424 424
Branches 69 69
=========================================
Hits 424 424
Continue to review full report at Codecov.
|
The tests seems to pass just fine in node "16", "15", "14" (don't know why we don't test >12.17 - we should be doing it also...) just one squiggle red line that works fine otherwise... may just be something VScode have problem to understand (fyi, v16 use built in streams and the lower nodejs versions use the polyfill) |
Just tough of one other possible idea for web-stream-polyfill: instead of Lines 5 to 12 in 46f0623
what if you @MattiasBuelens just pulled the same version from stream/web also if it exist? The benefit of the builtin one by NodeJS is that it's transferable (with PostMessage - i believe). and everybody would use the same and latest version all the time So isn't the built in much more preferred? all your variants (polyfill & ponyfill) would basically be |
Hmm, yeah, I was afraid of that... 😕 I'll have a look, but not sure if I'll get it to work.
It's definitely something I want to support at some point. It's the same idea as MattiasBuelens/web-streams-polyfill#20, but extended to Node. It's dangerous though. We definitely don't want to call
That said, we could potentially provide it as a separate package entry point, e.g. So yeah, at least for now, I suggest you do the |
yup, that is true |
Any progress with this PR? |
The purpose of this PR is:
To reduce file size.
This is what had to change:
This is what I like reviewers to know:
just b/c stream polyfill made a breaking change dose not mean we have to do it. The Blob API stays the same...
a patch release might be sufficient
I have just changed the way we import the polyfill
docs:
,fix(scope):
,feat(scope):
orbreaking(scope):
(Promised I would give a review and test the beta for Mattias)