Skip to content

fix: prefix stream/web import with node: #122

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

Merged
merged 1 commit into from
Oct 24, 2021

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Oct 15, 2021

The purpose of this PR is:

Making it more obvious that stream/web is a Node built-in. Since we upgraded to node-fetch 3.0 final our deploys are failing on Netlify (netlify/zip-it-and-ship-it#743). I'm hoping this would be part of a solution

This is what had to change:

Add node: prefix

This is what like reviewers to know:

This is a supported syntax for importing core modules. I believe the node: prefix is only available on Node 16. However, since stream/web is only available on Node 16 as well, I don't think that should be an issue. It does skip the require cache, which might cause some small performance loss - if the user's application is importing stream/web elsewhere I believe this will cause a new instance to be imported

I might wait to merge this until seeing what the reviewers of the detection package think about the idea as well (dependents/node-precinct#88). However, I thought I'd at least raise it here to see whether you'd be amenable to the idea.


  • 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
  • I updated the README.md
  • I Added unit test(s)

Copy link
Member

@LinusU LinusU left a comment

Choose a reason for hiding this comment

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

Neat 👍

@codecov
Copy link

codecov bot commented Oct 16, 2021

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff            @@
##             main      #122   +/-   ##
========================================
  Coverage        ?   100.00%           
========================================
  Files           ?         4           
  Lines           ?       425           
  Branches        ?        69           
========================================
  Hits            ?       425           
  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 9022f8f...07227e2. Read the comment docs.

@jimmywarting
Copy link
Contributor

node: prefix was added in v12.20 but only for esm syntax, require() got support for it later in v14.18

image

we still support v12.20

@jimmywarting
Copy link
Contributor

a better detection (on your end) would be to discard everything after a / so detecting built in modules such as stream/whatever is compared as just stream

@benmccann
Copy link
Contributor Author

node: prefix was added in v12.20 but only for esm syntax, require() got support for it later in v14.18

per the issue description, stream/web is only available on Node 16, so it's completely safe to use node: here

@jimmywarting
Copy link
Contributor

per the issue description, stream/web is only available on Node 16, so it's completely safe to use node: here

touché

@jimmywarting jimmywarting merged commit 836709f into node-fetch:main Oct 24, 2021
@benmccann benmccann deleted the node-prefix branch October 24, 2021 15:48
@benmccann
Copy link
Contributor Author

Thanks! Any chance we can get a new release with this included?

@jimmywarting
Copy link
Contributor

it didn't bring much new to the table that it was so much worth making a release out of. but i guess we could make a new patch release

i have found out that NodeJS now have added a Readable.toWeb and a FileHandle.readableWebStream() method that i would like to use to read local files. But FileHandle.readableWebStream don't take any start/stop options... so i would have to use Readable.toWeb instead.

@benmccann
Copy link
Contributor Author

A new patch release would be great. No project that's using fetch 3.0 final can deploy to Netlify, so it's a somewhat serious issue even if the fix here is very minor.

@jimmywarting
Copy link
Contributor

okey

@benmccann
Copy link
Contributor Author

great, thanks!

Netlify has merged the fix on their end (netlify/zip-it-and-ship-it#773), so all we need is a release here and then folks should be able to use this library on Netlify again

@jimmywarting
Copy link
Contributor

it's on my agenda, i where just thinking if maybe i could get #125 merged first. I would like for someone to cast just an eye on it first. After that i will make a new release

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants