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

Conversation

jimmywarting
Copy link
Contributor

@jimmywarting jimmywarting commented Oct 28, 2021

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. Import web/stream and then reassign the warning fn back to what it was

This 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 warning


  • I prefixed the PR-title with docs: , fix(area): , feat(area): or breaking(area):
  • I updated ./CHANGELOG.md with a link to this PR or Issue

@codecov
Copy link

codecov bot commented Oct 28, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@836709f). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 836709f...7e4a063. Read the comment docs.

@jimmywarting jimmywarting changed the title fix(warning) Supress warnings when importing experimental web stream from NodeJS fix(warning): Supress warnings when importing experimental web stream from NodeJS Oct 28, 2021
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.

@zdm
Copy link

zdm commented Oct 30, 2021

Best way:

  • load web/streams on demand only, when user requires blob with the steams support;
  • keep warning, so user can know, that he use unstable api;

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.

require "stream/web" produces warning
5 participants