Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

fix: Node.js 10 support #1358

Merged
merged 4 commits into from
May 30, 2018
Merged

fix: Node.js 10 support #1358

merged 4 commits into from
May 30, 2018

Conversation

jacobheun
Copy link
Contributor

@jacobheun jacobheun commented May 18, 2018

Opening this early to provide more visibility.

Update:
The jenkins builds are now running and working for node 10.0. There is an issue in node 10.1 with fs.promises warnings, mentioned below, but there is a fix already merged that should go out with the next release so I am going to leave that for now.

I will be working on getting all the dependency change PRs merged so I can get this PR finalized.

Things to fix:

Dependencies

The following PRs need to be completed prior to this release.

@ghost ghost assigned jacobheun May 18, 2018
@ghost ghost added the status/in-progress In progress label May 18, 2018
@jacobheun jacobheun force-pushed the fix/node10 branch 2 times, most recently from 7d9d586 to ff02c08 Compare May 21, 2018 18:38
@jacobheun
Copy link
Contributor Author

Node 10.2 was released yesterday, it includes a fix for Experimental fs.promise Warnings when they're not actually used that resulted in js-ipfs cli errors. Once all dependency PRs are merged and released I will update the dependencies in this PR.

We should be good for node 10 support.

@alanshaw
Copy link
Member

alanshaw commented May 29, 2018

@jacobheun dignifiedquire/lock-me#3 seems to have been merged and released - can I review this and get this merged?

src/core/boot.js Outdated
@@ -3,6 +3,7 @@
const waterfall = require('async/waterfall')
const series = require('async/series')
const extend = require('deep-extend')
const RepoErrors = require('ipfs-repo/src/errors')
Copy link
Member

Choose a reason for hiding this comment

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

This is an anti-pattern. Can we expose the repo errors through require('ipfs-repo').errors instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, and yes ipfs/js-ipfs-repo#171

package.json Outdated
@@ -110,10 +110,10 @@
"ipfs-block": "~0.7.1",
"ipfs-block-service": "~0.14.0",
"ipfs-multipart": "~0.1.0",
"ipfs-repo": "~0.20.0",
"ipfs-repo": "^0.22.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

We should keep the ~ for packages below 1.0.0, as stated in the js-ipfs guidelines.

Copy link
Member

Choose a reason for hiding this comment

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

good catch! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, my mistake, ran the wrong install command. New ci build on the way!

@jacobheun jacobheun requested a review from alanshaw May 29, 2018 19:12
@ghost ghost assigned daviddias May 30, 2018
@daviddias daviddias changed the title Fix node 10 build fix: Node.js 10 support May 30, 2018
@alanshaw
Copy link
Member

👀 ing now

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

This LGTM. npm install works locally and all the tests pass. Just waiting on the result from CI.

@ghost ghost assigned alanshaw May 30, 2018
@daviddias daviddias merged commit d394a12 into master May 30, 2018
@ghost ghost removed the status/in-progress In progress label May 30, 2018
@daviddias daviddias deleted the fix/node10 branch May 30, 2018 09:08
@alanshaw
Copy link
Member

@jacobheun could you setup commit signing for next time? https://help.github.com/articles/signing-commits-using-gpg/

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants