search

LeaVerou / multirange

A tiny polyfill for HTML5 multi-handle sliders
http://projects.verou.me/multirange/
MIT License
607 stars 82 forks source link

Expose API via CommonJS instead of a global variable, when supported #38

Closed markasoftware closed 5 years ago

markasoftware commented 5 years ago

Much nicer for us browserify folks.

LeaVerou commented 5 years ago

I was ready to merge this, then noticed the version change in package.json. Could you please revert that? Even if we do release a new version, this does not warrant such a major release.

Thanks for contributing!

markasoftware commented 5 years ago

Semver dictates that any breaking change should include a new major version — even if the change is small. Angular, for example, went from version 2 to 4 without any massive upheaval. If the major version is kept the same, unwitting consumers of this package will receive the new version automatically and their apps will break.

LeaVerou commented 5 years ago

Why is this a breaking change? Won't it still have a global when a global is desired?

markasoftware commented 5 years ago

CommonJS modules don't usually set globals, so I disabled the global when CommonJS modules are supported, for better or for worse.

LeaVerou commented 5 years ago

Yes but people couldn't use this as a module before, so how is it a breaking change? For people not using it as a module, it will still have the global...no?

markasoftware commented 5 years ago

What was the purpose of the npm package before this PR? As I understand it, it was to do things like this using browserify:

require('multirange')
multirange(myElement)

This would no longer function after this PR; you would need var multirange = require('multirange'). If you are fine with breaking the few apps that may have used it like the example at top, let me know and I'll revert the version.

LeaVerou commented 5 years ago

Hmm, nah, you convinced me, I guess the version bump makes sense. I'll merge it as-is. Thanks for contributing!