search

andyperlitch / jsbn

The jsbn library is a fast, portable implementation of large-number math in pure JavaScript, enabling public-key crypto and other applications on desktop and mobile browsers.
Other
167 stars 41 forks source link

Fix backwards incompatible change #8

Closed mscdex closed 7 years ago

darlanalves commented 8 years ago

Please see https://github.com/andyperlitch/jsbn/issues/6

I think the current syntax is better suited. On a parallel discussion, maybe SecureRandom is not in the right place and should be kept in a new module.

mscdex commented 8 years ago

@darlanalves I've already seen that issue. This is an easy backwards compatibility win. Would it help if I added BigInteger.BigInteger = BigInteger to be also 100% backwards compatible with the breaking commit?

darlanalves commented 8 years ago

Both would work, but not for semantic versioning. SecureRandom is a new feature, not a fix or a minor change, and should be in a new version of this package, or a new package that uses jsbn as a dependency. The current projects that are already using jsbn on version 0.0.0 would expect no API changes nor export format changes.

For the sake of clarity, I'd rather change the export syntax to allow the addition of both SecureRandom and BigIntenger in the exports object, then tag it v1.0.0, so other projects wouldn't break.

The current code state had no version change since the last format. That breaks every project out there using the latest version.

mscdex commented 8 years ago

Assuming this project would be using semver (I could not find it stated anywhere that this is the case), major version bumps are for breaking changes. I wouldn't call tacking on SecureRandom as an extra property on the exported object a breaking change because most people are just doing something like:

var BigInteger = require('jsbn');
var bn = new BigInteger('1234567890');

They're typically not iterating over the exported object and/or otherwise asserting/assuming anything about its properties. It would be a different story though if the breaking change was instead a removal of SecureRandom as a property or removing something from the BigInteger prototype for example.

Changing the exported object in a backwards incompatible way and tagging it as v1.0.0 is fine, but the point of this PR is to fix the breakage without removing SecureRandom and without having to bump the major version (especially since this is the only backwards incompatible change I'm aware of and it's trivial to fix).

mscdex commented 7 years ago

@andyperlitch Why was this closed?

andyperlitch commented 7 years ago

@mscdex Doing some catch-up as maintainer. I thought this was an outdated thread but if you still think we should provide backwards compatibility, I am for adding BigInteger.BigInteger = BigInteger as you said.

mscdex commented 7 years ago

Well FWIW this breaking change was the reason I chose to vendor this library (with this PR's fix) in my ssh2-streams module (also because I don't need the SecureRandom functionality).

If BigInteger.BigInteger = BigInteger is going to be added, it would have to be in addition to the changes in this PR, not in place of it, since the key is setting module.exports = BigInteger.

andyperlitch commented 7 years ago

True. I am thinking I should publish v0.1.1 with your changes here so as to adhere to semver and consider the backward-incompatibility of 0.1.0 a bug which is patched in 0.1.1. From v1.0.0 onward, I will instead opt for the more es6-friendly default export, a la #15.

I have created a branch called release-0.1, from which i will cut 0.1.1. Other backwards-compatible changes can occur there as well if desired. Thanks for the help and sorry for the lag.