Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

Update required Node version #937

Merged
merged 1 commit into from
Feb 4, 2019
Merged

Update required Node version #937

merged 1 commit into from
Feb 4, 2019

Conversation

filips123
Copy link
Contributor

Readme says that minimum Node.js version is 10. This is true because of some dependencies which also require version 10. However, in package.json file, minimum version is 8. This is incorrect, so I'm fixing this now.

Copy link
Contributor

@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.

@filips123 I wasn't aware there were modules in the dependencies that weren't Node.js 8 compatible yet - have you encountered issues?

@filips123
Copy link
Contributor Author

Travis CI test of my package which has this package as a dependency will fail. Some of js-ipfs-http-client dependencies (js-libp2p-crypto) have set Node.js 10 as the minimum. Here is my build in Node.js 8.

I didn't test this with yarn install --ignore-engines (because I don't have Node.js 8 installed), but maybe there aren't any issues. If this is true, js-libp2p-crypto needs to be informed to lower required Node.js version.

I would be great if Node.js 8 would be supported (if possible) because some still use it.

@alanshaw
Copy link
Contributor

alanshaw commented Feb 4, 2019

Ah yes, ipfs-http-client doesn't actually use libp2p-crypto it just re-exports it for people to use. I'd prefer it if people required it or used the pre-built browser bundle from a CDN if they need it. It would also make for a much smaller ipfs-http-client bundle.

This problem will be resolved by #915. Lets merge this for now and we can revert after #915 is merged.

@alanshaw alanshaw mentioned this pull request Feb 4, 2019
@alanshaw
Copy link
Contributor

alanshaw commented Feb 4, 2019

CI failure is unrelated.

@alanshaw alanshaw merged commit bfd71c2 into ipfs-inactive:master Feb 4, 2019
@filips123
Copy link
Contributor Author

Ok, but don't forget to fix #926 which added Node.js 10 requirement to README.

@andremedeiros
Copy link

@alanshaw is there anything I can help with bringing engine back to Node 8? It looks like the related PRs have already been merged, and I assume the offending library was taken out.

@alanshaw
Copy link
Contributor

@andremedeiros if you can do the work to verify the library is compatible with Node.js 8 and send a PR that would be great!

@michaelsbradleyjr
Copy link
Contributor

See PR #996.

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