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

fix(blob-stream): Bump major web stream polyfill v4 #116

Closed
wants to merge 1 commit into from

Conversation

jimmywarting
Copy link
Contributor

@jimmywarting jimmywarting commented Sep 7, 2021

The purpose of this PR is:

To reduce file size.

This is what had to change:

  • Needed to bump web-stream-polyfill to next major version

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


  • I prefixed the PR-title with docs: , fix(scope): , feat(scope): or breaking(scope):
  • I updated ./CHANGELOG.md with a link to this PR or Issue
  • I updated one unit test
  • Wait for v4 stable to be released. This PR is as of now just a draft/wip. (just want to see how CI handles the test)
    (Promised I would give a review and test the beta for Mattias)
    • update this PR with newest (stable) v4 of stream polyfill when released (blocks this PR)

@codecov
Copy link

codecov bot commented Sep 7, 2021

Codecov Report

Merging #116 (46f0623) into main (46476fa) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #116   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          424       424           
  Branches        69        69           
=========================================
  Hits           424       424           
Impacted Files Coverage Δ
streams.cjs 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 46476fa...46f0623. Read the comment docs.

@jimmywarting
Copy link
Contributor Author

jimmywarting commented Sep 7, 2021

to @MattiasBuelens

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...)
(edit: just added v12 to ci)

just one squiggle red line that works fine otherwise... may just be something VScode have problem to understand

image

(fyi, v16 use built in streams and the lower nodejs versions use the polyfill)

@jimmywarting jimmywarting marked this pull request as draft September 7, 2021 15:13
@jimmywarting
Copy link
Contributor Author

jimmywarting commented Sep 7, 2021

Just tough of one other possible idea for web-stream-polyfill: instead of
web-streams-polyfill and our conditional loader:

fetch-blob/streams.cjs

Lines 5 to 12 in 46f0623

if (!globalThis.ReadableStream) {
try {
Object.assign(globalThis, require('stream/web'))
} catch (error) {
// TODO: Remove when only supporting node >= 16.5.0
require('web-streams-polyfill/polyfill')
}
}
...checking and preferring the builtin one by NodeJS (so everybody uses the same and always the latest version (cuz of non-interchangeable streams))

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?
(...to avoid issues like this: MattiasBuelens/web-streams-polyfill#75 (comment) & MattiasBuelens/web-streams-polyfill#20)

all your variants (polyfill & ponyfill) would basically be globalThis.polyfill = require('stream/web') || your_entire_code

@MattiasBuelens
Copy link

just one squiggle red line that works fine otherwise... may just be something VScode have problem to understand

Hmm, yeah, I was afraid of that... 😕 I'll have a look, but not sure if I'll get it to work.

what if you @MattiasBuelens just pulled the same version from stream/web also if it exist?

your polyfill/ponyfill would basically be globalThis.polyfill = require('stream/web') || your_entire_code

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 require() in the browser, or risk any fancy Browserify-like bundler injecting its own polyfill in place of that require() call. Also, it might not always be desirable to use the native implementation, as explained here:

Ponyfills should avoid using native APIs, because potential bugs or differences in the native APIs will make such a ponyfill less robust (therefore defeating one of its main purposes).

That said, we could potentially provide it as a separate package entry point, e.g. web-streams-polyfill/native-or-polyfill. Or we try something with conditional exports, e.g. adding a node version for the /polyfill entry point.

So yeah, at least for now, I suggest you do the try { require('stream/web') } thingy manually. 😅

@jimmywarting
Copy link
Contributor Author

jimmywarting commented Sep 7, 2021

It's the same idea as MattiasBuelens/web-streams-polyfill#20, but extended to Node.

yup, that is true

@JeDaYoshi
Copy link

Any progress with this PR?

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.

3.0.0-beta.10 suddenly 6.77 MB in install size
3 participants