search

dylang / shortid

Short id generator. Url-friendly. Non-predictable. Cluster-compatible.
https://www.npmjs.org/package/shortid
Other
5.74k stars 258 forks source link

2.2.9 only a patch update? #122

Closed cncolder closed 6 years ago

cncolder commented 6 years ago

I have read commit history. That's totally refactor under this release.

ai commented 6 years ago

I using SemVer idea of public API. If public API was not changed, it is a patch. If new public API was added, it is minor. If public API was removed/changed, it is a major.

This is why I decided that it is a patch since it works exactly as before. This refactoring only fixed bugs.

But I agree that for before-SemVer versioning policy it should be a major, since ”a lot of work was done” :D.

Do you have any problem with 2.2.9?

cncolder commented 6 years ago

I use shortid for mongoose _id and qrcode content in some production deploy.

I will try the new version in my local environment now.

cncolder commented 6 years ago

Hey @ai . After a short test. I find a problem :(

shortid.isValid() use alphabet.get().replace() to build regexp. But alphabet is not initial until setCharacters.

> var shortid = require('shortid')
undefined
> shortid.isValid('shortid')     <-- shortid alphabet is undefined
TypeError: Cannot read property 'replace' of undefined
    at Function.isShortId [as isValid] (node_modules/shortid/lib/is-valid.js:10:21)
> shortid()
'aRg4aX6pb'
> shortid.isValid('shortid')    <-- shortid alphabet is 0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ_-
true
>
ai commented 6 years ago

Yeap, try 2.2.10. The issue should be fixed there.