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: prefix stream/web import with node: #122

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

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.

None yet

3 participants