search

holepunchto / hyperdrive

Hyperdrive is a secure, real time distributed file system
Apache License 2.0
1.86k stars 135 forks source link

Breaking API change on minor version! #314

Closed mixmix closed 3 years ago

mixmix commented 3 years ago

hey, just upgraded from 10.18.0 to 10.20.0

There seems to be a breaking change the API:

10.18.0 usage : hyperdrive(path, key, opts) 10.20.0 usage : new Hyperdrive(path, key, opts)

wanted to post about this because I trusted semver and wasted time checking other things and source code before realising what had happened :heart:

mixmix commented 3 years ago

went looking for a CHANGELOG.md in case there were any other things changed and doesn't seem like you have one. I've only just got into practice of adding these to more popular packages and really love the pattern - even if it's just a couple of bullet points. I find making changes to the CHANGELOG as part of the PR flow makes this pretty painless if you wanted to try it

andrewosh commented 3 years ago

Thanks @mixmix, agree that having a CHANGELOG.md for future API changes makes complete sense. It's a pattern we use elsewhere too (in Hypercore, for example), but it hasn't made its way here yet.

That said, while the README was updated to promote the new Hyperdrive(...) pattern, the module actually exports a HyperdriveCompat function that supports both hyperdrive(...) and new Hyperdrive(...), so old code should still work just fine.

mixmix commented 3 years ago

ideally it should, but I have tests which immediately threw "Hyperdrive is not a constructor" at me (this is how I knew there was a breaking change!)

thanks for the prompt reply. Love this ecosystem, it's been a real gift to our project

andrewosh commented 3 years ago

Ah sorry to hear that @mixmix -- let's get to the bottom of this. I noticed that we were lacking a test that includes both constructor types, so I added a simple one here: https://github.com/hypercore-protocol/hyperdrive/blob/0a32747ba5f03b060a4bc2d47c93e299a0daa5ff/test/basic.js#L322

CI just passed on that branch (https://travis-ci.org/github/hypercore-protocol/hyperdrive/builds/773994048), and it seems to run fine on latest (16), LTS, and 10. I've manually been testing with 12/14 on Linux without issue as well. Any chance you're using a different version? If you have any code snippets, those could help pin this down too.

mixmix commented 3 years ago

Ah, I was running 10.18.0 and using the 10.20.0 docs

So while your api is backwards compatible, the docs are slightly misleading.

I assumed I should use new when in fact I could not in 10.18.0 because it wasn't supported there.

This is a funny case re semver change, not sure what's best. Happy to close this

andrewosh commented 3 years ago

Gotcha yeah that makes more sense! The README only reflects the latest version right now, and yeah that's a hard problem in general. PRs welcome there if you have any ideas for improving that.