uxsolutions / bootstrap-datepicker

A datepicker for twitter bootstrap (@twbs)
Apache License 2.0
12.66k stars 6.07k forks source link

Make conform to new UMD specification. #1761

Open EvanCarroll opened 8 years ago

EvanCarroll commented 8 years ago

The new UMD specification can be found here,

This would change the require code to be required like this in CommonJS

require('bootstrap-datepicker')();

but, it'll add the ability to make rendering work in PhantomJS,

require('bootstrap-datepicker')(document.window);

And, it would add the ability to share an instance of jQuery with another module (rather than having multiple instances of jQuery as the current code does). This is a tremendous advantage for Browserify users,

require('bootstrap-datepicker')(document.window, jQuery);

If you'd like I can submit a pull request.

acrobat commented 8 years ago

A pr would be great! It could also be included in 1.6 release planned for the end of the month!

But is it backwards compatible with the current implementation?

EvanCarroll commented 8 years ago

But is it backwards compatible with the current implementation?

Nope, there is no way to make it backwards compat. The old method of UMD did something very, very nasty and should have never been the standard. For Common.js, it included a new version of jQuery for every plugin that used require("module"). Well, that's majorly problematic because every plugin with jQuery is supposed to play well with every other plugin. So it just adds file size and a redundant copy of jQuery. The new version permits you to have one copy of jQuery shared, and each plugin would return an instance of jQuery..

Observe,

var $ = require('jquery');
require('bootstrap-datepicker')(document.window, $);
require('notifyjs')(document.window, $);

Or, even,

var $ = require('bootstrap-datepicker')();
require('notifyjs')(document.window, $);

You can see the code requiring a new version of jquery here in the bootstrap-datepicker repo..

EvanCarroll commented 8 years ago

Then again, I don't see you documenting CommonJS inclusion anywhere on this module, but undoubtedly someone is using it somewhere under Browserify or some other CommonJS build system.

acrobat commented 8 years ago

Hmm I was hoping that we could achieve BC in some way! I will take a look at the patch otherwise this has to wait until 2.0.

EvanCarroll commented 8 years ago

I'm going to put my own branch into production then. Please take a look at cherry picking the other changes out (which fix some issues I had with jslint/jscs) for your 1.6x release. And, also perhaps you should fork off a 2.x branch so this PR doesn't get lost? May want to milestone "2.x" this in github.

acrobat commented 8 years ago

I've added the milestone! But I will take a look at the patch in detail one of the days as we can't break BC in 1.x releases but I understand this changes should be adapted soon!

EvanCarroll commented 8 years ago

One other question about the packaging of bootstrap-datepicker. The root package.json in the repository points to dist/. This is actually a pretty bad practice (imho) for this reason here. I want to fork this module and run the fork in production. However, in order to that I have to regenerate dist. That's not a problem, but you probably don't every PR to have a new copy of dist because that guarantees that no two PR can be committed without a merge conflict.

I believe this can be remedied by

  1. putting an independent package.json into dist.
  2. using npm publish dist rather than npm publish.

This would allow those of forking to use our copy in production without having to push up our own dist/.

There are other ways we can handle this too, such as forking on the committers side with a copy of dist. But, I think that's kind of sloppy.

acrobat commented 8 years ago

Hmm don't know about that issue! Can you put this issue/description in a seperate issue? I will take a look at it and try to fix it! Thanks for the report!

Glutnix commented 8 years ago

I would like to know where this is at, as it is affecting me right now.

EvanCarroll commented 8 years ago

Patch is here,

https://github.com/eternicode/bootstrap-datepicker/pull/1762