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(warning): Supress warnings when importing experimental web stream from NodeJS #125

Merged
merged 4 commits into from
Oct 30, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ project adheres to [Semantic Versioning](http://semver.org/).
(Unreleased)
==================
### Removed
-
-
### Changed
-
-
### Added
-
### Fixed
Expand All @@ -19,6 +19,8 @@ project adheres to [Semantic Versioning](http://semver.org/).
- File name are now casted to string [#109]
- Slicing in the middle of multiple parts added more bytes than what what it should have [#109]
- Prefixed `stream/web` import with `node:` to allow easier static analysis detection of Node built-ins [#122]
- Added `node:` prefix in `from.js` as well [#114]
- Suppress warning when importing `stream/web` [#114]

## v3.1.2

Expand Down Expand Up @@ -102,3 +104,4 @@ project adheres to [Semantic Versioning](http://semver.org/).

[#108]: https://github.com/node-fetch/fetch-blob/pull/108
[#109]: https://github.com/node-fetch/fetch-blob/pull/109
[#114]: https://github.com/node-fetch/fetch-blob/pull/114
6 changes: 3 additions & 3 deletions from.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {statSync, createReadStream, promises as fs} from 'fs';
import {basename} from 'path';
import {MessageChannel} from 'worker_threads';
import {statSync, createReadStream, promises as fs} from 'node:fs';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if I'd necessarily prefix everything with node: as it introduces a small performance overhead. It is useful for the import of stream/web which is not available on all versions of Node though so that it can be more easily detected as a Node import by static analysis tools run by Netlify so that their deployments succeed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as it introduces a small performance overhead.

Hmm, do you have a source on this? I think that it's the new recommended way since the Node.js documentation is switching over to the node:-style.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was basing that statement off the fact that the docs say it skips the require cache: https://nodejs.org/api/modules.html#modules_core_modules

I don't know what if any performance impact that has, but I had assumed there would be some small impact

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the node: prefix...

Copy link

@andersk andersk Dec 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benmccann Bypassing the require cache makes it faster, not slower, since core modules are not added to the require cache either way.

import {basename} from 'node:path';
import {MessageChannel} from 'node:worker_threads';

import File from './file.js';
import Blob from './index.js';
Expand Down
56 changes: 35 additions & 21 deletions streams.cjs
Original file line number Diff line number Diff line change
@@ -1,37 +1,51 @@
/* c8 ignore start */
// 64 KiB (same size chrome slice theirs blob into Uint8array's)
const POOL_SIZE = 65536;
const POOL_SIZE = 65536

if (!globalThis.ReadableStream) {
// `node:stream/web` got introduced in v16.5.0 as experimental
// and it's preferred over the polyfilled version. So we also
// suppress the warning that gets emitted by NodeJS for using it.
try {
Object.assign(globalThis, require('node:stream/web'))
const process = require('node:process')
const { emitWarning } = process
try {
process.emitWarning = () => {}
Object.assign(globalThis, require('node:stream/web'))
process.emitWarning = emitWarning
} catch (error) {
process.emitWarning = emitWarning
throw error
}
} catch (error) {
// TODO: Remove when only supporting node >= 16.5.0
// fallback to polyfill implementation
Object.assign(globalThis, require('web-streams-polyfill/dist/ponyfill.es2018.js'))
}
}

try {
const {Blob} = require('buffer')
// Don't use node: prefix for this, require+node: is not supported until node v14.14
// Only `import()` can use prefix in 12.20 and later
const { Blob } = require('buffer')
if (Blob && !Blob.prototype.stream) {
Blob.prototype.stream = function name(params) {
let position = 0;
const blob = this;
Blob.prototype.stream = function name (params) {
let position = 0
const blob = this

return new ReadableStream({
type: 'bytes',
async pull(ctrl) {
const chunk = blob.slice(position, Math.min(blob.size, position + POOL_SIZE));
const buffer = await chunk.arrayBuffer();
position += buffer.byteLength;
ctrl.enqueue(new Uint8Array(buffer))
return new ReadableStream({
type: 'bytes',
async pull (ctrl) {
const chunk = blob.slice(position, Math.min(blob.size, position + POOL_SIZE))
const buffer = await chunk.arrayBuffer()
position += buffer.byteLength
ctrl.enqueue(new Uint8Array(buffer))

if (position === blob.size) {
ctrl.close()
}
}
})
}
}
if (position === blob.size) {
ctrl.close()
}
}
})
}
}
} catch (error) {}
/* c8 ignore end */