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: remove upper bound for node engine semver #49

Merged
merged 3 commits into from
Jun 11, 2020

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 [email protected] 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?

@@ -20,7 +20,7 @@
"node-fetch"
],
"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 [email protected] and deprecated [email protected].

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fails to install due to node engine version
4 participants