-
Notifications
You must be signed in to change notification settings - Fork 32
fix(warning): Supress warnings when importing experimental web stream from NodeJS #125
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
Codecov Report
@@ Coverage Diff @@
## main #125 +/- ##
========================================
Coverage ? 100.00%
========================================
Files ? 4
Lines ? 448
Branches ? 73
========================================
Hits ? 448
Misses ? 0
Partials ? 0 Continue to review full report at Codecov.
|
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'; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
Best way:
|
The purpose of this PR is:
... To suppress the warning message when ppl include fetch-blob in their projects
This is what had to change:
... i override
process.emitWarning
with a noop-fn. Importweb/stream
and then reassign the warning fn back to what it wasThis is what I like reviewers to know:
The test are also importing web/stream's so a warning is still emitted while developing. just running
node index.js
don't emit any warningdocs:
,fix(area):
,feat(area):
orbreaking(area):