-
Notifications
You must be signed in to change notification settings - Fork 31
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
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):