AMD with concatenated JS file #13812

Closed
hnrch02 opened this Issue Jun 13, 2014 · 27 comments

Projects

None yet

6 participants

@hnrch02
Member
hnrch02 commented Jun 13, 2014

The problem is that you can only have one define call within the same file, unless you specify a name that substitutes the file name which would normal determine the name of the module. This is currently not the case with our UMD code and makes the concatenated JS file not work with AMD.

I did some more tinkering with this and came to the conclusion that we have two options:

  1. We process the source files in the concatenation step to replace part of the UMD code to look like this:
typeof define == 'function' && define.amd ? define('bs.<PLUGIN_NAME>', ['jquery'], o_o)

This would solve the problem, but create a new one: You'd need to require all plugins individually like so:

require(['jquery', 'bs.affix', 'bs.alert', ...], function ($) {
  !!$.fn.affix // -> true
})
  1. We process the source files in the concatenation step to replace part of the UMD code to look like this:
typeof define == 'function' && define.amd && typeof require == 'function' ? require(['jquery'], o_o)

Then you'd require Bootstrap like this and you'd be good to go:

require(['jquery', 'bootstrap'], function ($) {
  !!$.fn.affix // -> true
})

I prefer option 2, but I don't know if that has downsides. I can't think of any, as we're not exporting anything that could be used in the require call by the end user.

/cc @fat @cvrebert @XhmikosR

@hnrch02
Member
hnrch02 commented Jun 13, 2014

Also, RequireJS doesn't seem to like how we declare these modules, using either of the two methods, it doesn't wait for the file to load and execute.

This is rather complex from what I can tell while working on some AMD examples/tests... I say we revert UMD in master for now, put it in a separate branch and punt it to 3.3. I image that 3.2 must be long overdue now and shipping UMD with a major problem like this doesn't seem like a good idea. Also, Bootstrap can be easily used with RequireJS right now without the definitions, it just needs some shimming.

@cvrebert cvrebert added the js label Jun 13, 2014
@hnrch02 hnrch02 referenced this issue Jun 17, 2014
Closed

UMD tests #13843

@hnrch02
Member
hnrch02 commented Jun 17, 2014

If fixed this, although in a rather hacky way, see hnrch02@060f3fc.

@hnrch02 hnrch02 added a commit to hnrch02/bootstrap that referenced this issue Jun 17, 2014
@hnrch02 hnrch02 Fix for AMD when using concatenated file; fixes #13812 060f3fc
@cvrebert cvrebert added this to the v3.2.0 milestone Jun 17, 2014
@hnrch02 hnrch02 added a commit to hnrch02/bootstrap that referenced this issue Jun 18, 2014
@hnrch02 hnrch02 Fix for AMD when using concatenated file; fixes #13812 229fb59
@hnrch02 hnrch02 added a commit to hnrch02/bootstrap that referenced this issue Jun 18, 2014
@hnrch02 hnrch02 Fix for AMD when using concatenated file; fixes #13812 65dff0b
@cvrebert cvrebert modified the milestone: v3.3.0, v3.2.0 Jun 23, 2014
@cvrebert cvrebert added a commit that referenced this issue Jun 23, 2014
@cvrebert cvrebert Revert UMD (#13772 & friends) for now, due to #13812.
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
c2c19a4
@lagden
lagden commented Jun 24, 2014

why?
broke my project!!!
I have to make a shim again!!! what the problem?!

@lagden
lagden commented Jun 24, 2014

Which branch or tag is using AMD?

@XhmikosR
Member

None.

@lagden
lagden commented Jun 24, 2014

😥
Forked and solved!!

But you should create a new tag (v3.1.2) and not replace the current release! I think!

@hnrch02
Member
hnrch02 commented Jun 24, 2014

3.1.1 did not include anything UMD related, you were using bleeding edge code and that comes with the danger of us removing stuff we previously added.

@XhmikosR
Member

Exactly what @hnrch02 says. It will be implemented properly in the next release.

@lagden
lagden commented Jun 24, 2014

oh yeah!! I cloned from the master branch... sorry...
I will waiting for release...

🍻

@lagden
lagden commented Jun 24, 2014

by the way...
the UMD was working like a charm!!
😄

@hnrch02
Member
hnrch02 commented Jun 24, 2014

My tests concluded something else, please give more insight into your setup.

@lagden
lagden commented Jun 24, 2014

This is the small project: https://github.com/lagden/textela
I created this project to make some mockups. It use AngularJS + TwBs

I use VoloJS to set libraries and r.js to optimize the project.

There's nothing special!!

Here is my config file:
https://github.com/lagden/textela/blob/master/dev/js/config.js

I used dropdown and modal: http://tex.lagden.in/#!/telas/show/inicial

Setting the library on package.json:

Defining on module /dev/js/modules/telas/telas.js:


To dev:
Run: npm install and grunt server

Grunt server running on /dev folder

To optimize:

  • tools/build.js replace requirejs by almond

Run: grunt build
This will generate a folder build for deploy


My README.md is outdated.

Some requirements are:

gem install compass --pre
gem install bootstrap-sass
npm install -g requirejs
npm install -g volo
npm install -g grunt-cli
npm install -g jade
npm install -g autoprefixer

It might help!!

@hnrch02
Member
hnrch02 commented Jun 24, 2014

I don't think you understand the problem this issue is describing. You are requiring some of our plugins individually, not using all of them via the concatenated file.

@lagden
lagden commented Jun 24, 2014

I understand...
https://github.com/umdjs/umd

tooltip load jquery
popover load tooltip that load jquery before

I'll do it this way:

// tooltip.js
(function(factory) {
    if (typeof define === 'function' && define.amd) {
        // AMD
        define(['jquery'], factory);
    } else {
        // Browser globals
        factory(jQuery);
    }
}(function($) {
    //... some code
    $.fn[pluginName] = function(options) {
        return this;
    };
}));

// popover.js
(function(factory) {
    if (typeof define === 'function' && define.amd) {
        // AMD
        define(['tooltip'], factory);
    } else {
        // Browser globals
        factory(jQuery);
    }
}(function($) {
    //... some code
    $.fn[pluginName] = function(options) {
        return this;
    };
}));

// concat and compressed
(function(e){if(typeof define==="function"&&define.amd){define(["jquery"],e)}else{e(jQuery)}})(function(e){e.fn[pluginName]=function(e){return this}});(function(e){if(typeof define==="function"&&define.amd){define(["tooltip"],e)}else{e(jQuery)}})(function(e){e.fn[pluginName]=function(e){return this}})
@hnrch02
Member
hnrch02 commented Jun 24, 2014

That's where this problem comes in. Per the AMD spec there must only ever be one define call in any given file, unless the first argument is a string specifying the ID of the module.

@hnrch02
Member
hnrch02 commented Jun 24, 2014

Though that would still require configuration on the side of the end-user.

@lagden
lagden commented Jun 24, 2014

Yeap... I never seen other way...

About bundles:
requirejs/requirejs#1034 (comment)

@hnrch02
Member
hnrch02 commented Jun 24, 2014

Reading all the resources, Bundles seem to fix the problem but don't really solve it. RequireJS would accept our file, the user would still need to require each and every one of our plugins manually.

@jaridmargolin

I personally love how Lodash handles the issue.

Core
Common
AMD

I believe most of the magic is happening here. May be worth investigating.

@hnrch02
Member
hnrch02 commented Jun 30, 2014

Well, the problem described in this issue was fixed by #13843, but before that was merged everything related to UMD was reverted. I don't know how @cvrebert @XhmikosR and @fat wanna go about this.

@jaridmargolin

@hnrch02 - #13843 This makes the assumption that you are going to just plug and play the entire library. Most likely however, users consuming the library via module system, common or amd, are looking to incorporate individual modules.

UMD should be incorporated on the final concatenated file, however this is only half of the problem. Pointing back to (Lodash):

Core - implements a UMD wrapper (this is the file found on cdn's etc...)
Common - A pure commonjs module build
AMD - A pure amd module build

@hnrch02
Member
hnrch02 commented Jul 1, 2014

@jaridmargolin Not sure I'm following, this would have been totally possible in a CommonJS environment:

var $ = require('jquery')
require('bootstrap/js/modal')

$('#myModal').modal()

and in AMD:

define(['jquery', 'bootstrap.modal'], function($) {
  $('#myModal').modal()
})

The only thing that was missing from these individual UMD definitions was the dependency of certain plugins on transition.js. Browse the code before UMD was removed here.

@jaridmargolin

Apologies for my last comment. I looked at master, which currently does not contain the following snippet in each module:

(function (o_o) {
    typeof define  == 'function' && define.amd ? define(['jquery'], o_o) :
    typeof exports == 'object' ? o_o(require('jquery')) : o_o(jQuery)
  })(function ($) {

and I looked at the changed files referenced in #13843. Between the two it appeared to me that you were just throwing a giant UMD wrapper around the concatenated file and calling that good enough.

Should have reviewed a little more thoroughly before commenting.


Back to Lodash (last time I promise). By creating three different distributions, you remove the complexity of trying to have one solution that works in multiple environments. No if/else logic or unnecessary boiler in each file. Just a single build process.

@hnrch02
Member
hnrch02 commented Jul 1, 2014

I see how that's appropriate for something like Lo-Dash with their very fancy build system and complex structure but I think for our purpose the boilerplate in each file that gets stripped if plugins are concatenated will do the job. But that's just me, we need @fat's @cvrebert's and @XhmikosR's opinion on this.

@lagden
lagden commented Jul 1, 2014

hauhauhau... awesome!!!

@mdo
Member
mdo commented Aug 2, 2014

Punting to the v4 checklist.

@mdo mdo closed this Aug 2, 2014
@mdo mdo removed this from the v4.0.0 milestone Aug 2, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment