Skip to content

Conversation

pimlie
Copy link
Contributor

@pimlie pimlie commented Jun 11, 2020

Resolves: #48

/cc @tinovyatkin

@codecov
Copy link

codecov bot commented Jun 11, 2020

Codecov Report

Merging #49 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@ Coverage Diff @@ ## master #49 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 1 1 Lines 183 183 Branches 30 30 ========================================= Hits 183 183 

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 126b4ec...e694966. Read the comment docs.

@readeral
Copy link

Interesting node-fetch changed the engine requirements to be >=10.17.0 but with the linting issues, maybe the more specific version you've landed on is required by fetch-blob?

@tinovyatkin
Copy link
Member

tinovyatkin commented Jun 11, 2020

@pimlie What do you see as a downside of just >=10.17?

@tinovyatkin
Copy link
Member

@xxczaki It's a serious bug, we will need to release a patch version after merging this (as it may preventing install of node-fetch@3.beta7 via yarn on Node LTS)

Copy link
Member

@tinovyatkin tinovyatkin left a comment

Choose a reason for hiding this comment

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

I would love to see this in sync with current version of node-fetch - do you see any problem with versions 11.x to be included?

],
"engines": {
"node": "^10.17.0"
"node": "^10.17.0 || >=12.3.0"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"node": "^10.17.0 || >=12.3.0"
"node": ">=10.17"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what I added at first btw, but changed it due to the lint error. Not sure if v11 also supports it, I trusted the lint error to specify the correct minimum version

@xxczaki
Copy link
Member

xxczaki commented Jun 11, 2020

@tinovyatkin We will also need to release a new beta version of node-fetch with the updated fetch-blob dependency (maybe something like 3.0.0-beta.7-fixnodeversion), won't we?

@tinovyatkin
Copy link
Member

@xxczaki No, node-fetch is fine, we have caret there, so, it will pick up 2.0.1 automatically. We should, however, deprecate 2.0.0 of fetch-blob

@pimlie
Copy link
Contributor Author

pimlie commented Jun 11, 2020

@tinovyatkin See the first lint error, it said: The stream.Readable.from is not supported until Node.js 12.3.0 (backported: ^10.17.0).

@xxczaki xxczaki merged commit 1d1faeb into node-fetch:master Jun 11, 2020
@xxczaki
Copy link
Member

xxczaki commented Jun 11, 2020

cc @tinovyatkin @pimlie

Released fetch-blob@2.0.1 and deprecated fetch-blob@2.0.0.

@tinovyatkin
Copy link
Member

@pimlie Thank you for your contribution!

@pimlie pimlie deleted the fix-node-engine-semver branch June 11, 2020 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants