Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make conform to new UMD specification. #1761

Open
EvanCarroll opened this issue Jan 19, 2016 · 10 comments · May be fixed by #1762
Open

Make conform to new UMD specification. #1761

EvanCarroll opened this issue Jan 19, 2016 · 10 comments · May be fixed by #1762

Comments

@EvanCarroll
Copy link

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
Copy link
Member

acrobat commented Jan 19, 2016

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
Copy link
Author

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
Copy link
Author

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.

@EvanCarroll EvanCarroll linked a pull request Jan 19, 2016 that will close this issue
@acrobat
Copy link
Member

acrobat commented Jan 20, 2016

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
Copy link
Author

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 acrobat added this to the 2.0.0 milestone Jan 20, 2016
@acrobat
Copy link
Member

acrobat commented Jan 20, 2016

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
Copy link
Author

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
Copy link
Member

acrobat commented Jan 20, 2016

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
Copy link

Glutnix commented Apr 21, 2016

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

@EvanCarroll
Copy link
Author

Patch is here,

#1762

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants