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

Added support for AMD/CommonJS/Globals. #12909

Closed
wants to merge 4 commits into from

Conversation

pherrymason
Copy link

Pull request related to #12783

@VirtueMe
Copy link

VirtueMe commented Mar 4, 2014

Lots of linting exception.

@cvrebert
Copy link
Collaborator

cvrebert commented Mar 4, 2014

@raulferras Per the contribution guidelines, please use 2-space indents instead of tabs.

@pherrymason
Copy link
Author

Sure.

@cvrebert cvrebert added this to the v3.2.0 milestone Mar 4, 2014
Added transition dependency to AMD definitions.
@jenssimon
Copy link

The "transition" dependencies won't work. The dependencies must not end with ".js".

So

(function (factory) {
  if (typeof define === 'function' && define.amd) {
    define(['jquery', './transition.js'], factory)
  } else if (typeof exports === 'object') {
    var jQuery = require('jquery')
    require('./transition.js')
    factory(jQuery)
  } else {
    factory(this.jQuery)
  }
}(function ($) {

should be replaced by

(function (factory) {
  if (typeof define === 'function' && define.amd) {
    define(['jquery', './transition'], factory)
  } else if (typeof exports === 'object') {
    var jQuery = require('jquery')
    require('./transition')
    factory(jQuery)
  } else {
    factory(this.jQuery)
  }
}(function ($) {

in

  • alert.js
  • carousel.js
  • collapse.js
  • modal.js
  • tab.js
  • tooltip.js

@pherrymason
Copy link
Author

Ok, fixed.
It worked to me under CommonJS as webpack allows it.

@mokkabonna
Copy link
Contributor

Hope this finally is going to make it in. Right now I can't even use jquery with AMD (with noconflict) because of:

jqueryCheck: 'if (typeof jQuery === \'undefined\') { throw new Error(\'Bootstrap\\\'s JavaScript requires jQuery\') }\n\n',

@pherrymason
Copy link
Author

I don't know of AMD, but with CommonJS (webpack) I'm currently doing this:

var $ = require('jquery');
window.jQuery = $;
require('bootstrap');

This does it for me.

@zlatanvasovic
Copy link
Contributor

@raulferras You should use jQuery = $, without the window..

@pherrymason
Copy link
Author

That's true for bootstrap specifically, but other jQuery plugins require explicitly to overwrite window.jQuery, it all depends of the plugins/libs you're going to use.

@cvrebert
Copy link
Collaborator

Running cross-browser tests on Travis... https://travis-ci.org/twbs/bootstrap/builds/20557300

@cvrebert
Copy link
Collaborator

Browser tests pass!: https://travis-ci.org/twbs/bootstrap/jobs/20557303 😄

@cvrebert
Copy link
Collaborator

We should also add moduleType to bower.json after merging this. (Ref: bower/bower#934)

@cvrebert
Copy link
Collaborator

@twbs/team: So, aside from needing to test this manually/interactively in a few browsers beforehand, are there any objections to merging this PR?

@mdo
Copy link
Member

mdo commented Mar 13, 2014

I have zero objections as I know nothing about this. We'll need @fat to weigh in.

@fat
Copy link
Member

fat commented Mar 17, 2014

yes. i object

@fat
Copy link
Member

fat commented Mar 17, 2014

this stuff should be added at build time… dont think we should put it on the raw plugins

@fat
Copy link
Member

fat commented Mar 17, 2014

i think ryan florence wrote a really complete version of this at one point… maybe we could use that to add at compile time?

@cvrebert
Copy link
Collaborator

@pherrymason
Copy link
Author

If this is added at build time, will we be allowed to build the different plugins indepently?

@cvrebert
Copy link
Collaborator

@raulferras It'd produce individual per-plugin "module-ized" JS files, if that's what you mean.

@pherrymason
Copy link
Author

yes, great then.

@fat
Copy link
Member

fat commented Mar 23, 2014

the more i think about this… the more I think that getting into the business of being other projects build systems seems like a bad idea.

I think as a guiding rule, it's generally safer to lean towards raw components, then trying to include all the hardware to connect to every 3rd party lib under the sun.

thoughts?

@zlatanvasovic
Copy link
Contributor

Agree.

2014-03-23 2:51 GMT+01:00 Jacob [email protected]:

the more i think about this… the more I think that getting into the
business of being other projects build systems seems like a bad idea.

I think as a guiding rule, it's generally safer to lean towards raw
components, then trying to include all the hardware to connect to every 3rd
party lib under the sun.

thoughts?


Reply to this email directly or view it on GitHubhttps://github.com//pull/12909#issuecomment-38370499
.

Zlatan Vasović - ZDroid

@enricostano
Copy link

status? guide lines? any example out there? thanks!

@fat
Copy link
Member

fat commented Jun 10, 2014

closing in favor of #13772

@XhmikosR XhmikosR closed this Jun 10, 2014
fat pushed a commit that referenced this pull request Jun 10, 2014
fat pushed a commit that referenced this pull request Jun 11, 2014
fat added a commit that referenced this pull request Jun 11, 2014
cvrebert added a commit that referenced this pull request Jun 23, 2014
Will hopefully revert this reversion and land a fully-working version of UMD in v3.3.0.

Revert "some changes from #13801 - add strict mode back and =="
This reverts commit 2b302f6.

Revert "Fix regression of #10038 introduced by #13772"
This reverts commit e9d6756.

Revert "MD/CommonJS/Globals #12909"
This reverts commit 1c6fa90.

Revert "address #13811"
This reverts commit f347d7d.

Conflicts:
    js/carousel.js
    js/collapse.js
    js/dropdown.js
    js/modal.js
    js/tab.js
    js/tooltip.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants