Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

AMD support #1029

Closed
wants to merge 4 commits into from
@rxaviers
Collaborator

http://wiki.jqueryui.com/w/page/66995889/AMD-Support

  • Remove build from Grunt
  • Remove "globals" from all files
  • Script to rename all files
  • Actually rename all files, removing the "jquery.ui." prefix, before landing this PR
  • Copy updated i18n files to datepicker demos

After merging to master:

Skip

ui/jquery.ui.core.js
((14 lines not shown))
-var uuid = 0,
+var coreUuid = 0,
@rxaviers Collaborator

Some "global" variables need to be renamed, so they don't clash in the final bundle jquery-ui.js (because, each file UMD wrapper gets removed for optimization).

Shall we rename them case-by-case like the above, or shall we be more programmatic and follow some kinda pattern through all the files that prevents them from clashing? Eg. by using key-value pairs objects:

var core = {
  uuid: 0,
  ...
};
@scottgonzalez Owner

Neither. The variables shouldn't be global. We'll need to just address these on a case-by-case basis.

@rxaviers Collaborator

Ok, all the current clashing ones have been renamed here 13be9a0

@jzaefferer Owner

Scott, do you want those globals to be addressed as part of this PR? Or handle them later?

@rxaviers Collaborator

We're mimicking the build process of modernizr, which uses requirejs to merge the parts into the bundle; plus it processes the files, so each individual AMD wrapper (in our case UMD) is removed.

We need to address those globals here, or we'll have a problematic bundle js. :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Gruntfile.js
((30 lines not shown))
+ exclude: [ "jquery" ],
+ create : true
+ }],
+ wrap: {
+ start: createBanner() + "(function( $ ) {",
+ end: "})( jQuery );"
+ },
+ onBuildWrite: function ( id, path, contents ) {
+ if ( (/define\([\s\S]*?factory/).test( contents ) ) {
+ // Remove UMD wrapper
+ contents = contents.replace( /\(function\( factory[\s\S]*?\(function\( \$ \) \{/, "" );
+ contents = contents.replace( /\}\)\);\s*?$/, "" );
+ }
+ else if ( (/^require\(\[/).test( contents ) ) {
+ // Replace require with comment `//` instead of null string, because of the mysterious semicolon
+ contents = contents.replace( /^require[\s\S]*?\]\);$/, "// mysterious semicolon: " );
@rxaviers Collaborator

Making sure this gets attention of review.

@jzaefferer Owner

Can you explain this? I don't quite understand what's going on. Where do we have required in our source code?

@rxaviers Collaborator

Sure... Follow my comments above. The onBuildWrite function is executed for each file that requirejs optimizer processes, ie. main.js, plus all the UI components. The require is in the generated main.js.

@rxaviers Collaborator

The "mysterious semicolon" is a weird thing caused (I guess) by the optimizer. Because, if we inspect the contents before and after. We see that it is an empty string after.

@rxaviers Collaborator

Just let me know if you still have any question.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.accordion.js
@@ -12,7 +12,19 @@
* jquery.ui.core.js
* jquery.ui.widget.js
*/
-(function( $, undefined ) {
+(function( factory ) {
+ if ( typeof define === "function" && define.amd ) {
+ // AMD. Register as an anonymous module.
+ define([
+ "jquery",
+ "jqueryui/jquery.ui.core",
@jzaefferer Owner

Did we discuss the naming at all? Do you have any references of other frameworks that export multiple modules and have a similar naming scheme?

@rxaviers Collaborator

Nope, we haven't. I'm following jrburke's https://github.com/jrburke/jqueryui-amd

@scottgonzalez Owner

@jrburke @gfranko @slexaxton Any comments on how we should be naming these?

@rxaviers Collaborator

For naming suggestions different than this one, please also comment on two things:

@jrburke
jrburke added a note

I do not have full context here, but in general, for a collection of modules that are in a directory, relative IDs should be used for dependencies. That allows consumers of the code to place the code in a named directory of their choosing. So if the files are like so:

  • ui/jquery.ui.accordion.js
  • ui/jquery.ui.core.js

Then accordion would use "./jquery.ui.core" to refer to the core.js module.

@gfranko
gfranko added a note

I agree with @jrburke that the naming should be consistent with the relative file paths.

I also want to bring up that I think the Widget Factory should be exported as a named module. Lot's of jQuery plugins authors use the Widget Factory to construct their plugins and need a way to natively provide full AMD support so that users do not have to use the shim configuration. I think all of the other jQueryUI files though, besides the Widget Factory, should be anonymous modules.

@scottgonzalez Owner

@gfranko Wouldn't it be a path configuration, not a shim? I wouldn't count on us doing a named module for anything other than jQuery core.

@rxaviers Collaborator

Thanks, @jrburke. It turns out that I didnt follow your approach then :P. Now, I have update it according to your comment.
Question: Does using relative IDs show any counter effect than using a regular path, or it's completely equivalent?

@rxaviers Collaborator

@gfranko, like @scottgonzalez said, I guess you won't need to use shim here, since we're defining each component with its appropriate AMD define.

Please, take a look at https://github.com/jrburke/jqueryui-amd#configuring-amd-loading and just let me know if you still find a problematic case writing plugins?

@gfranko
gfranko added a note

@scottgonzalez @rxaviers You can set a path configuration for jQueryUI plugins and it will work, but I don't think that it will work for third-party plugins. Take my SelectBoxIt (third-party) Widget Factory plugin example:

;(function (selectBoxIt) {

    if (typeof define === "function" && define.amd) {
        // AMD. Register as an anonymous module.
        define(["jquery"], function() {

            selectBoxIt(window.jQuery, window, document);

        });
    } else {
        // Browser globals
        selectBoxIt(window.jQuery, window, document);
    }

}
(function ($, window, document, undefined) {

    // SelectBoxIt code goes here

})); // End of all modules

Since there is no way that SelectBoxIt could know what moduleID to use for the Widget Factory, I couldn't list the Widget Factory as a dependency. This means that a user would have to do this:

shim: {
  selectboxit: ["jquery.ui.widget"]
}

This is what I wish I could do:

        // AMD. Register as an anonymous module.
        define(["jquery", "jquery.ui.widget"], function() {

            selectBoxIt(window.jQuery, window, document);

        });
@scottgonzalez Owner

Why is it unreasonable to just assume all 3rd party plugins will use "jquery.ui.widget" as the name? Even if they didn't, users of selectboxit could still handle this through a path setting.

@gfranko
gfranko added a note

I don't see many users actually defining named modules in their code. Most people just use the paths property that you mention inside of their require config to set up alias names. Like this:

// Require.js Configurations
// -------------------------
require.config({
  // 3rd party script alias names (Easier to type "jquery" than "libs/jquery, etc")
  paths: {
      // Core Libraries
      // --------------
      "jquery": "../libs/jquery",
      // Plugins
      // -------
      "text": "../libs/plugins/text"
  }
}

So, if the Widget Factory registers as an anonymous AMD module and third-party plugins list jquery.ui.widget as a dependency, then a user MUST set their Widget Factory path name to jquery.ui.widget. If a user tries to user any other path name, then Require.js will throw an error that it cannot find the jquery.ui.widget dependency when it loads the third-party plugin.

Here is an example that would cause an error:

// Require.js Configurations
// -------------------------
require.config({
  // 3rd party script alias names (Easier to type "jquery" than "libs/jquery, etc")
  paths: {
      // Core Libraries
      // --------------
      "jquery": "../libs/jquery",
      "widgetfactory": "../libs/jquery.ui.widget",
      // Plugins
      // -------
      "text": "../libs/plugins/text",
      "selectboxit": "../libs/plugins/selectboxit"
  }
}

// Throws an error that it cannot find the jquery.ui.widget dependency that SelectBoxIt lists
require(["jquery", "widgetfactory", "selectboxit"], function($) {
  $('select').selectBoxIt();
});

If the Widget Factory is exported as a named AMD module, then the previous example would still work even though the user chose their own distinct path name for the Widget Factory. Here is what the Widget Factory export would look like:

(function( factory ) {
  if ( typeof define === "function" && define.amd ) {
    // AMD. Register as an anonymous module.
    define( "jquery.ui.widget", [ "jquery" ], factory );
  } else {
    // Browser globals
    factory( jQuery );
  }
}(function( $ ) {
})); // End

This provides a better user experience in my opinion. But like you said, if a user had set up the correct jquery.ui.widget path, then the previous example would have worked.

@jrburke What do you think?

@rxaviers Collaborator

@gfranko this is how a user application could be configured to use a third party plugin and jquery-ui:

 |- components
 |    |- jqueryui
 |    |    |- jquery.ui.widget.js
 |    |- jquery
 |    |    |- jquery.js
 |    |- foo
 |    |    |- foo.js
 |- main.js

main.js:

require.config({
  paths: {
    jquery: "components/jquery/jquery",
    jqueryui: "components/jqueryui",
    foo: "components/foo/foo"
  }
}

require(["foo"], function() {
  // My app that uses the foo 3rd party plugin
});

components/foo/foo.js

define(["jquery", "jqueryui/jquery.ui.widget"], function($) {
  // Your 3rd-party plugin, eg selectboxit
});

Does it make sense? Any trouble? To make it easier, there are projects whose goal is to wire-up installed Bower components into your RequireJS config.

@gfranko
gfranko added a note

Your example assumes that there is a jqueryui module with that exact name. If there is not a jqueryui module, then my plugin would fail because Require.js would not be able to find it.

@scottgonzalez Owner

I'll ask again why anyone would use a name other than jquery.ui.widget. Even if they did, why can't they just specify the path for jquery.ui.widget? I don't see how defining a path for widgetfactory would prevent this. Your documentation for selectboxit wouldn't change.

I feel like nobody using AMD should be using jqueryui unless they're defining that themselves. If you're tracking dependencies, you should be doing it for real and only loading what you need. bower install jqueryui will not give you a full build, it'll give you individual files that you can load via AMD or directly via script tags.

@gfranko
gfranko added a note

@scottgonzalez Someone would set a different path name because it is not immediately obvious to everyone what jquery.ui.widget is. I (and you) would know that it was the widget factory, but others may not and would want to change the name.

I agree that you should only load what you need, but you will find a ton of people who will load all of jqueryui and not dynamically load the individual pieces. A major benefit of a tool, like Require.js, is that you can dynamically load files, but lot's of people use it purely as a file decoupling/organization tool.

Regardless of whether the Widget Factory is exported as an anonymous or named module, this is great =)

@jrburke
jrburke added a note

I do not feel like I am following the 'jqueryui' vs 'jquery.ui.widget' reference discussions, here is my understanding on how this code would be delivered for AMD projects:

The developer gets a folder of the jQuery UI modules into their AMD project (could be via a package manager or just manual unzip). That folder is named something. It does not really matter what the name is (developer could choose any name), but pretend they call it 'jqueryui'. Although, a specific folder name suggestion is probably a good idea so the developer consuming widgets made by others all use the same ID. So I suggest encouraging the 'jqueryui' name, or some standard name of your choosing.

Now, the developer wants to use jquery.ui.widget.js in their AMD project. They would either set the path for 'jqueryui' to that jqueryui folder or place the folder in the folder used for baseUrl.

To refer use the widget factory, they should use the 'jqueryui' prefix for all modules in that directory. So, they would use dependency IDs like 'jqueryui/jquery.ui.widget' and 'jqueryui/jquery.ui.autocomplete'. This all works out and is good.

If someone is suggesting the code would be consumed differently than above, then that is probably the place where I get lost.

Side note: if you wanted to make things shorter for AMD users, since all the files are already in a folder name (the 'jqueryui' mentioned in this example), then the files inside that directory can just be names like widget.js and autocomplete.js. I understand though if that is not possible for other reasons, just suggesting a benefit resulting from the IDs all being suffixes under the folder name.

@rxaviers Collaborator

@gfranko I understood you are concerned about your 3rd-party plugins using jquery-ui as a dependency (eg. widget) vs. how your plugin dependencies could be transparent to the user application. But, I guess you can't. It's analogous to jquery-ui requiring jquery.

@gfranko
gfranko added a note

@jrburke Basically, lot's of jQuery plugins use the jQueryUI Widget Factory completely separate of jQueryUI to help build standalone jQuery plugins (not associated with jQueryUI at all). The jQueryUI Widget Factory removes the normal boilerplate code a developer has to write for every jQuery plugin and adds other helpful things (methods, etc) as well.

I was suggesting that the jquery.widget.js code would be consumed by developers who don't necessarily use jQueryUI. So third-party developers, like me, who use the jQueryUI Widget Factory to build standalone jQuery plugins could use some sort of dependency hook. Like this:

;(function (selectBoxIt) {

    if (typeof define === "function" && define.amd) {
        // AMD. Register as an anonymous module.
        define(["jquery", "jquery.ui.widget"], function() {

            selectBoxIt(window.jQuery, window, document);

        });
    } else {
        // Browser globals
        selectBoxIt(window.jQuery, window, document);
    }

}
(function ($, window, document, undefined) {

    // The SelectBoxIt jQuery plugin code would go here

})); // End of all modules

Note: @scottgonzalez has mentioned that in the future he might pull the jQueryUI Widget Factory out of the jQueryUI repo.

@rxaviers Collaborator

@scottgonzalez how likely are we to change filenames? (jqueryui/jquery.ui.autocomplete.js -> jqueryui/autocomplete.js)?

@jrburke
jrburke added a note

@gfranko thanks for the explanation. So I think it works out: for jQuery UI files that do not have other dependencies (besides jQuery), then it should be possible to use that file on its own, and named/referred to as whatever the developer wants, if jquery.ui.widget.js does not name itself.

If the developer gets into a situation where they pick up some modules that want a full jQuery UI checkout, then the map config could be used to point 'jqueryui/jquery.ui.widget' to the standalone jquery.ui.widget.js used by the developer's other code.

@rxaviers Collaborator

@SlexAxton :+1: awesome structure

@gfranko
gfranko added a note

@jrburke So if my jQuery plugin calls the define() method with two dependencies ( jquery, jquery.ui.widget ), like this:

        define(["jquery", "jquery.ui.widget"], function() {

            selectBoxIt(window.jQuery, window, document);

        });

How would a user use the map config to set up the jquery.ui.widget dependency?

@jrburke
jrburke added a note

@gfranko if that module depends on 'jquery.ui.widget', but the project has a full jQuery UI installed at 'jqueryui', then the map config would look like:

map: {
 '*': {
    'jquery.ui.widget': 'jqueryui/jquery.ui.widget'
  }
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Gruntfile.js
((9 lines not shown))
+ }) ),
+ dest: "dist/tmp/main.js"
+ }
+ },
+ requirejs: {
+ all: {
+ options: {
+ dir: "dist/build",
+ appDir: "ui",
+ baseUrl: ".",
+ optimize: "none",
+ optimizeCss: "none",
+ paths: {
+ "jquery": "../jquery-1.9.1",
+ "jqueryui": ".",
+ "main" : "../dist/tmp/main" // FIXME replace to <%= %>
@rxaviers Collaborator

This main.js is generated on pre-requirejs. This file's main goal is to include all the components we bundle in the build process by requireing them as AMD dependencies.

PS: A test I need to make is if we can skip this file and, instead, use the modules' include parameter for this same purpose.

@rxaviers Collaborator

About FIXME, this string could be deduced from this template "../<% print( preRequirejs.all.dest.replace( /\\.js/, \"\" ) ) %>". But, I think it's way too complex and I wouldn't suggest that...

@rxaviers Collaborator

Just for the record: I tested my above PS: I tried to use modules' include instead of using the main file, and it did NOT work. The include does not calculate dependencies correct (as done with require). The resulting bundle duplicates some dependencies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Gruntfile.js
((17 lines not shown))
+ appDir: "ui",
+ baseUrl: ".",
+ optimize: "none",
+ optimizeCss: "none",
+ paths: {
+ "jquery": "../jquery-1.9.1",
+ "jqueryui": ".",
+ "main" : "../dist/tmp/main" // FIXME replace to <%= %>
+ },
+ /* try use include: [] instead adding dist/tmp/main with require(...) */
+ modules : [{
+ name : "jquery-ui",
+ include : [ "main" ],
+ exclude: [ "jquery" ],
+ create : true
+ }],
@rxaviers Collaborator

This will generate a file called dist/build/jquery-ui.js containing all the dependencies of main.js (ie. all the components bundled) excluding the jquery file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Gruntfile.js
((99 lines not shown))
grunt.registerTask( "default", [ "lint", "test" ] );
grunt.registerTask( "lint", [ "jshint", "csslint", "htmllint" ] );
-grunt.registerTask( "test", [ "qunit" ] );
-grunt.registerTask( "sizer", [ "concat:ui", "uglify:main", "compare_size:all" ] );
-grunt.registerTask( "sizer_all", [ "concat:ui", "uglify", "compare_size" ] );
-
-// "copy:dist_units_images" is used by unit tests
-grunt.registerTask( "build", [ "concat", "uglify", "cssmin", "copy:dist_units_images" ] );
+grunt.registerTask( "test", [ "copy:dist_units_images", "qunit" ] );
+grunt.registerTask( "sizer", [ "concat:ui", "uglify:bundle", "compare_size:bundle" ] );
+grunt.registerTask( "sizer_all", [ "concat:ui", "uglify", "cssmin", "compare_size" ] );
+grunt.registerTask( "build", [ "clean", "preRequirejs", "requirejs", "postRequirejs", "clean:distGarbage", "jshint:bundleJs", "uglify:bundleJs", "uglify:bundleI18n", "concat:css", "cssmin:bundle" ] );
@rxaviers Collaborator

I am skipping jshint:bundleI18n, since we haven't linted them so far and they have a lot of lint errors. We need to remember to include this after I18n files are linted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tjvantoll
Collaborator

Hey @rxaviers. I gave this branch a shot and only noticed one thing:

require([ "./jquery.ui.autocomplete" ], function( autocomplete ) {
    autocomplete === undefined; // true
});

Seems like the autocomplete widget should be available here.

The only other thing I noticed is that the...

 * Depends:
 *  jquery.ui.core.js
 *  jquery.ui.widget.js
 *  jquery.ui.position.js
 *  jquery.ui.menu.js

comments in the code are redundant now. Should we remove them?

@rxaviers
Collaborator

Seems like the autocomplete widget should be available here.

It shouldn't, because autocomplete or any other component doesn't return anything, they just extend $. You should do this:

require.config({
  paths: {
    jquery: "path/to/jquery",
    jqueryui: "path/to/jquery-ui"
  }
});

require([ "jquery", "jqueryui/jquery.ui.autocomplete" ], function( $ ) {
    $.fn.autocomplete === undefined; // should be false
});

Does it make sense?

@rxaviers
Collaborator

comments in the code are redundant now. Should we remove them?

Indeed they are redundant. I vote to remove them :+1:

@scottgonzalez

Regarding the return value for the module, @arschmitz and I are already planning on making the widget factory return the generated constructor. Once we do that, we should be able to just have:

return $.widget( "ui.plugin", { ... } );

The dependency comments should go away.

@jrburke

I meant to respond to some comments around named modules, but looks like they are now classified as "outdated". Just to confirm then:

Named modules (define() calls with the first argument a string for that module's ID) should be avoided. If all of these modules are in a directory, say called 'jqueryui', then if that directory is either placed at the AMD loader's baseUrl, or if there is a paths config set up for 'jqueryui' then it should all work out. Consumers of these modules would use IDs like 'jqueryui/jquery.ui.autocomplete' to refer to these modules.

@rxaviers
Collaborator

@jrburke,

jquery-ui depends on jquery. When defining the components, we do define(["jquery", ...]). So, it turns out we're supposed to let users know they should define the following in their config to use jquery-ui with AMD.

require.config({
  paths: {
    jquery: "path/to/jquery"
  }
});

Unless they have a jquery.js file with that exact name on his baseUrl. Right? (double checking)

@jrburke

@rxaviers that is correct, either a paths config for 'jquery' or 'jquery.js' in the baseUrl.

There are fancier options like map config, but for single file JS dependencies, just paths config or ideally just dropping jquery.js in the baseUrl should be the encouraged routes.

@gfranko

@jrburke Here is the link for the other discussions: #1029 (comment)

@rxaviers
Collaborator

@jrburke Excellent! Thanks.

It would be awesome if you could bless these two files: (so, I am safe this change is good)

PS: basically, I'd like you to check the define and possibly the UMD wrapper for jQuery plugins.

@jrburke

@rxaviers those files look fine to me.

@rxaviers
Collaborator

thanks!

@tjvantoll
Collaborator

Regarding the return value for the module, @arschmitz and I are already planning on making the widget factory return the generated constructor. Once we do that, we should be able to just have:

return $.widget( "ui.plugin", { ... } );

@scottgonzalez Excellent. I would expect (and want) that.

@jzaefferer
Owner

Making $.widget return the constructor is pretty easy to implement, just return the constructor: https://gist.github.com/jzaefferer/7b9236a9b9fabb9d3aff (also welcome to the tautology club)

Might as well implement that as part of this PR.

@jzaefferer
Owner

At the meeting yesterday we discussed that the build task here is just for size comparisons, since everything else moved to DB. It looks like a lot of this PR would be unnecessary without that build task. Not that the work has been a waste of time, but the actual building needs to be implemented in DB, not here, right?

Maybe we should land that cleanup and the widget-returns-constructor change before this PR, then rebase.

Gruntfile.js
((84 lines not shown))
}
});
+grunt.registerMultiTask( "postRequirejs", "Strip define call from dist file", function() {
+ this.filesSrc.forEach(function( filepath ) {
+ // Remove `define("main" ...)` and `define("jquery-ui" ...)`
+ var contents = grunt.file.read( filepath ).replace( /define\("(main|jquery-ui)", function\(\)\{\}\);/g, "" );
+
+ // Remove the mysterious semicolon `;` character left from require([...]);
+ contents = contents.replace( /\/\/ mysterious semicolon.*/g, "" );
@jzaefferer Owner

Why does the regex literally contain "mysterious semicolon"?

@rxaviers Collaborator

Makes sure I remove the right thing.. But, the idea is to figure out why this is happening and fix the root cause :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
build/tasks/build.js
@@ -131,8 +131,4 @@ grunt.registerMultiTask( "copy", "Copy files to destination folder and replace @
}
});
-grunt.registerTask( "clean", function() {
@jzaefferer Owner

Switching to grunt-contrib-clean probably shouldn't be part of this PR. If we split out other things, include this as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.autocomplete.js
@@ -14,7 +14,21 @@
* jquery.ui.position.js
* jquery.ui.menu.js
@jzaefferer Owner

You're going to remove these comments, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.dialog.js
@@ -17,7 +17,24 @@
* jquery.ui.position.js
* jquery.ui.resizable.js
*/
-(function( $, undefined ) {
+(function( factory ) {
+ if ( typeof define === "function" && define.amd ) {
+ // AMD. Register as an anonymous module.
+ define([
+ "jquery",
+ "./jquery.ui.core",
+ "./jquery.ui.widget",
+ "./jquery.ui.button",
+ "./jquery.ui.draggable",
+ "./jquery.ui.mouse",
+ "./jquery.ui.position",
+ "./jquery.ui.resizable"
@jzaefferer Owner

So we always require these optional dependencies?

It looks like we have to, since requirejs has no concept of optional dependencies. The workaround doesn't look like it would work for us.

@scottgonzalez Owner

We should probably not include optional dependencies. If you want a draggable dialog, you should include draggable.

@rxaviers Collaborator

If optional dependencies are not included, feature is simply ignored.

if ( this.options.draggable && $.fn.draggable ) {
    this._makeDraggable();
}
if ( this.options.resizable && $.fn.resizable ) {
    this._makeResizable();
}

If we are not requiring these two dependencies, shall we throw an error if user picks a feature whose dependency is not loaded (eg. this.options.draggable && !$.fn.draggable)?

@rxaviers Collaborator

Is there any other component with the same need?

@scottgonzalez Owner

I don't see any reason to change what we're doing and start throwing an error. The API docs have dependency sections for each widget.

@tjvantoll Collaborator

I don't see any reason to change what we're doing and start throwing an error. The API docs have dependency sections for each widget.

Agreed.

Is there any other component with the same need?

  • Datepicker / Tabs / Tooltip optionally depend on effects core.
  • Spinner optionally depends on Globalize.
@rxaviers Collaborator

If I use AMD to require dialog and if it simply ignores stuff, I would not guess what's going on at all, unless we state that very clearly that user needs to load more stuff in order to dialog to work.

Previously, user had to load all dependencies. So, it would be more clear that he needed to load those optional ones or nothing. But, with AMD I expect a component to handle its own dependencies or at least let me know that I need to take action.

@scottgonzalez Owner

Previously, user had to load all dependencies.

No you didn't. You may have chosen to do so if you were just using a full build, but you certainly didn't have to. If we put them in the define() call, they're no longer really optional. If we throw an error, then we're saying that the defaults aren't valid. We've had this design for years and it's worked well for us.

@rxaviers Collaborator

thanks for the list @tjvantoll.

@rxaviers Collaborator

Should we keep "optional dependencies" in the comments?

@scottgonzalez Owner

No, we have real documentation for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@scottgonzalez

Please don't make the return value part of this PR. It will need to happen much sooner than this is going to land. I will land it when @arschmitz has finished a proof of concept in the Mobile repo.

@scottgonzalez

When the rest of the discussion settles down, we need to talk about tracking CSS dependencies. But I don't want to discuss that until there are fewer other discussions going on.

@rxaviers
Collaborator

At the meeting yesterday we discussed that the build task here is just for size comparisons, since everything else moved to DB. It looks like a lot of this PR would be unnecessary without that build task. Not that the work has been a waste of time, but the actual building needs to be implemented in DB, not here, right?

Maybe we should land that cleanup and the widget-returns-constructor change before this PR, then rebase.

Right, the build would be "moved" to DB. We'd only land the new UMD wrapped source files (knowing they work). Will let you guys know when it's updated.

@rxaviers
Collaborator

The biggest cons of removing the build task is that we are not able to jshint the final output on each commit anymore. Therefore, it's hard to know whether variables among components might clash.

@rxaviers
Collaborator

DownloadBuilder is ready to build these UMD files jquery/download.jqueryui.com#156

@bleathem bleathem referenced this pull request in richwidgets/richwidgets
Open

Implement the widgets in an AMD/require.js compatible manner #16

@jzaefferer
Owner

There are four jquery.ui.datepicker.*.js files under demos/datepicker. We should update those copies to match the other source files.

@scottgonzalez

There are four jquery.ui.datepicker.*.js files under demos/datepicker.

I've created a checklist in the PR description to track the open issues since the comment thread is getting pretty long.

@scottgonzalez

@slexaxton @gfranko @arschmitz @timmywil

How should we handle CSS dependencies? Should they be included in the source and/or dist files? Or should we be doing custom tracking of CSS dependencies outside of the define()?

@gfranko

I think you will have to do custom tracking of CSS dependencies outside of the define() method, since AMD loaders don't natively support loading CSS files.

@jzaefferer
Owner

While hard to impossible to apply here as-is, the approach for specifying CSS as a dependency outlined in this blog post might be interesting: http://backstage.soundcloud.com/2012/06/building-the-next-soundcloud/

@gfranko

@jzaefferer That's an interesting technique, but hard to apply to jQueryUI (as you said).

@spadgos

@jzaefferer yeah, we're still using this technique on soundcloud.com. The above article explains the idea of converting the CSS to modules, but it didn't quite explain the next step of adding them to the page. Since each module defines/exports a <style> tag, we simply check if that element is in the document (by testing for el.parentNode). If yes, then nothing needs to be done, if no, then add it to the <head>.

Overall, this works really well and has been a great way of managing all sorts of dependencies. There's only two issues:

  • IE has limitations on the number of <style> tags it will parse. IE9's limit is 32; I think IE10+ may have removed the limitation; I'm not sure about IE8 and below. We get around this by creating a single style tag and appending the new style's text into it.
  • Each time a style tag is added, it causes the browser to recalculate styles. This isn't optimal, but overall it's not a massive bottleneck. It only happens once per style tag per session, and is only a couple of milliseconds.
@spadgos

Also, this is the code in our build step to do the conversion:

function convertCSS(reqPath) {
  // converts "views/sound/sound.css" to its filesystem path
  var path = requireToPath(reqPath),
      contents = fs.readFileSync(path, 'utf8');

  return [
    'define("',
    reqPath,
    '",[], function (require, exports, module) {\n',
    ' var style = module.exports = document.createElement("style"),',
    '     data = ', quoteString(contents), ';\n',
    ' if (style.styleSheet) {\n',
    '   style.styleSheet.cssText = data;\n',
    ' } else {\n',
    '   style.appendChild(document.createTextNode(data));\n',
    ' }\n',
    '});\n'
  ].join('');
}
@rxaviers
Collaborator

@spadgos please correct me if I'm wrong. But, I understood that in your view script, you declare the dependent CSSes with its paths eg. define(["path/to/file.css"], function() {...});, and before this is ever called, you register a named define eg. define("path/to/file.css", [], function() {...}) that instead of returning anything, it's a script that handles styling the page with the appropriate CSS data.

@spadgos

Well, there's two different cases -- during development, we're using requirejs and dynamically loading each module as it is required; and during production (for the purposes of this discussion) all files are combined and served at once, and we use almondjs as the AMD loader.

During development, calling require('path/to/file.css') will actually request a file from our dev server called path/to/file.css.js. This is converted on the fly using the code above.

During production, in the combined javascript file, there's a module defined using the code above:

define("views/sound/title.css", [], function(require, exports, module) {
  var style = module.exports = document.createElement("style"),
    data = '.here { is: "the css" } #that we.write {}';
  if (style.styleSheet) {
    style.styleSheet.cssText = data;
  } else {
    style.appendChild(document.createTextNode(data));
  }
});

And then when it's getting added to document:

  insert : function (css) {
    if (!_.isArray(css)) {
      css = [css];
    }
    css.forEach(function (node) {
      // snip out code to handle IE <= 9's style tag limitations
      if (node.parentNode) {
        return;
      } else {
        head.appendChild(node);
      }
    });
  }

does that answer your question?

@rxaviers
Collaborator

Yes, @spadgos thank you.

@rxaviers
Collaborator

One slightly different approach could be done by using a css plugin in the lines of what @SlexAxton does in his hbs plugin, using a similar logic to include the style in the page as done by @spadgos with the additional benefit of not needing to create one .js "loader" per css file during development.

Side comment: two existing css plugins could also be evaluated: https://github.com/tyt2y3/requirejs-css-plugin/ and https://github.com/guybedford/require-css.

@rxaviers
Collaborator

Handling CSS dependencies this way is a step forward modularity, which is good, and perhaps could even be extended to templates that we currently inline/embed in the code. It could be a pre-web-components solution for script, template, and style modularity.

Said that, there are some issues we'll face. Because, we have chosen to make UI sources consumable as is (reason why we're adding an UMD wrapper on the sources instead of simply adding an AMD wrapper and then building something UMD in a later step). Picking any of the CSS dependency solutions above, we'll enforce them to the users as well (or requiring the plugin as a dependency, or requiring the soundclound-environment-like constrain).

Can we handle CSS via AMD + not enforce them to users? I guess this would only be possibly if we had sources the way we want (could be AMD only), then having a built step to produce the UMD files without CSS dependency definitions.

@scottgonzalez

I think we'll want to do the comment implementation similar to jQuery Mobile.

@arschmitz
Collaborator

i have to say having worked with it the comment implementation while not perfect seems to suite our needs better then any of these other methods we have not really add any trouble at all with the css part of our setup.

@jzaefferer
Owner

So for someone loading jQuery UI via bower and requirejs, how would they deal with CSS dependencies? Can they use the source comments as well, or is that only useful for our build process? Obviously I know very little how this works in jQuery Mobile, even after digging around it's Gruntfile and tasks.

@arschmitz
Collaborator

in mobile the comments are only for the build process.

@gfranko

Is jQueryUI moving towards converting files to AMD modules internally (like jQuery Core) and then building with the RequireJS Optimizer and removing any AMD trace? Or is jQueryUI just following UMD patterns to be consumable by other developers who are using AMD?

If it is just exposing a consumable UMD pattern, I don't see how you can list CSS dependencies in the optional define() method, since many AMD devs are not using CSS plugins or their own special logic (e.g. SoundCloud) to include CSS as a JS dependency.

@scottgonzalez

We're going to be using UMD for source files and for the built file.

@gfranko

Gotcha, then it make sense to have some sort of implementation to remove the CSS dependencies in the build process. What is the comment-based approach that jQuery Mobile is using to do this?

@arschmitz
Collaborator

this is the comment format we use in mobile https://github.com/jquery/jquery-mobile/blob/master/js/widgets/listview.js#L6 and these comments are parsed by https://github.com/gseguin/node-amd-builder

@spadgos

I'm just an observer in this repo, but I'll just say that when you're moving to proper modular code with explicit dependencies, using code comments for some dependencies (because they are dependencies, just as much as the rest of the code) is the wrong approach. require() is powerful and explicit -- consider, for example, that you could remap certain require paths for a mobile-specific build.

For the same reasons that you aren't using specially formatted comments to tie together your javascript source modules, I think all the same arguments would apply to avoid doing that for templates and css too.

@scottgonzalez

@spadgos But that assumes that everyone who wants to use AMD is using an implementation that supports CSS.

@gfranko

What @scottgonzalez said. It's not an ideal approach to use comments, but the ecosystem doesn't provide first-class support for listing CSS/HTML as AMD dependencies yet.

@SlexAxton
@gfranko

@SlexAxton So are you advocating to explicitly define CSS dependencies in the conditional define() because all of the major AMD loaders can support a CSS plugin?

Or are you saying that you should use a build technique, like the one SoundCloud is using, to build all CSS dependencies into JS modules?

@SlexAxton
@gfranko

At first glance, I like the idea of CSS dependencies being explicitly defined alongside the JS dependencies (very readable).

Unfortunately, if CSS is loaded async after an AMD loader is already loaded, you are likely to see a Flash of Unstyled Content (http://stevesouders.com/hpws/css-fouc.php).

Like @spadgos said, every CSS file that is loaded causes browser style recalculations.

In my opinion, the best solution is to list the CSS file dependencies in the define() methods during development, and then strip them out during the build process. I also think you should make sure that only JS files are listed as dependencies for the UMD pattern, which allows jQueryUI to be consumed by AMD users.

Just my two cents.

@spadgos

Just to be clear, the CSS source is a javascript file after the build has done its thing. It's a module exactly like any other one, and it exports a style element.

@rxaviers
Collaborator

Let's break this question down into smaller ones.

1) How to process the CSS or embed the styles?

I'd say the "how to process the CSS" doesn't really matter here (eg. if it's going to be inlined in a named modue javascript code, cause browser blinks when embedded, or if a build process will concatenate all the CSS'es, and minify them into a bundle). It does matter for jQuery UI user applications though (be it a project that uses jQuery UI, or be it jQuery UI's Download Builder for example). Each will chose what's best for its application. What has been raised here so far is of great value, and perhaps this discussion will continue in other places: our download builer, or a css plugin implementation for example.

Giving @spadgos and @SlexAxton's experience, plus the existing css plugins (even that none seems to be official first-class css plugins as @gfranko remembered), it seems enough anyway for knowing it is possible to be done and move one.

2) Handle CSS dependencies via AMD defines or comments?

I tend to agree with something... Although "handling CSS dependencies with comments" prevents us from adding dependencies to the project, it doesn't actually help much --- as @jzaefferer pointed out with his question, comments don't help AMD users, it only "helps" our build process (with very specific/adhoc code logic to handle that); and as @spadgos opinioned, defining such dependencies with AMD is more robust. It empowers users to overcome settings eg. CSS paths, or even how to process the CSS'es (I will talk about it again below). Which I agree with.

3) Should jQuery UI add an AMD dependency?

As @SlexAxton mentioned: "treat it like any other dependency".

Considering the benefits are enough, can we diminish the dependency impact? I mean, can we make it not mandatory?

Let's assume we handle CSS dependencies with AMD. It would be good for users that choose to ignore it, to be able to. Can we make it optional for users that don't want to handle CSS dependencies via AMD (as it is today, or as if implemented with comments). Nope, as commented earlier, choosing AMD will require AMD-users to take action. But, can we make it skippable? Yes, and it doesn't require too much action from users. See apendix [1] example.

The few extra step for AMD-users that don't want to handle CSS dependencies could be outweighed by the benefits for the ones who does --- including our download builder. :smiley:

So, the question is: Are we ok by adding this AMD dependency? It will affect AMD-users only, and (as I tried to show) users that don't care with CSS dependency can skip it (although, not ignore it) with few AMD configuration.

4) Should it be an AMD plugin?

Are we ok using an AMD plugin (eg. css!path/of/style.css) if we decide to handle the CSS dependencies via AMD? In my opinion, using a plugin gives users more control, because they can overcome a CSS path or the plugin itself with configuration.

5) Etc

Are there any concerns we should ask now that we could be skipping? I don't think of any...

Apendix 1:

Let's suppose we define our CSS dependencies by using an AMD plugin called css, the accordion widget would look something like this:

// jquery.ui.accordion.js
define([
    "jquery",
    "./jquery.ui.core",
    "./jquery.ui.widget",
    "css!jquery.ui.accordion.css"
], factoryFn );

Core would look like this:

// jquery.ui.core.js
define([
    "jquery",
    "css!jquery.ui.core.css"
], factoryFn );

Users that don't want to care about CSS dependencies could define a "null" css plugin, eg. by using text (or a more appropriate "null" one).

// User code app.js
requirejs.config({
    paths: {
        css: "bower_components/requirejs-text/text.js"
    }
})

One interesting note is about themes. By using the above code, themes are specified by the end-user on his app.js with:

define([
    "jquery",
    "jquery/ui/accordion",
    "css!my_theme.css",
], function($) {
...
});

Download Builder can use a similar approach to include a theme when it creates the bundles using rjs. (instead of the adhoc code we use today)

@scottgonzalez

@csnover What are your thoughts on this?

@csnover

I’d recommend doing what dgrid does, which is similar but not exactly the same as the css plugin proposal. Using this loader module, dependent CSS files are dynamically injected as dependencies, but only if those CSS files have not already been loaded by hand. This enables users to build all their CSS into a single file for deployment, without having to worry about broken CSS during development, and without building the CSS into JavaScript and doing multiple <style> tag injections which could break IE.

@jzaefferer
Owner

We've discussed this at the team meeting in Amsterdam. None of the solutions that load CSS through AMD are in a stage where we're willing to commit to them. In other words, specifying CSS dependencies through AMD is far from a 'solved problem', so we're not going to adopt that approach until it works.

Since we want to share the download builder between UI and Mobile as soon as possible, we'll adopt Mobile's approach of specifying CSS dependencies in comments in JS files. We'll port the implementation from their build (or download builder). Mobile used this long enough and never ran into any major problems. Once a better solution emerges, we can delete those comments and adopt the new solution.

@rxaviers can you go ahead with implementing that? If you have any questions about the implementation in Mobile, ask @gseguin or @arschmitz

@rxaviers
Collaborator

@jzaefferer Let's use jquery/download.jqueryui.com#177 as a follow-up to the "share the download builder between UI and Mobile" discussion.

@jzaefferer
Owner

I've put together a script to take care of the renames. We can apply that before we land this PR, to avoid breaking other existing PRs.

@rxaviers can you work on handling the CSS dependencies as discussed?

@rxaviers rxaviers referenced this pull request in jquery/download.jqueryui.com
Closed

Handle CSS dependencies from JS comments #178

@rxaviers
Collaborator

@jzaefferer About handling the CSS dependencies, do we need to change anything here besides adding the dependency-comments? Do demos, tests, or any other code need change?

UI Download Builder will be updated to:

@jzaefferer
Owner

As long as we don't use an AMD loader for our demos and tests, I don't think there's anything we need to change here. Even if we were, without a good CSS plugin we still wouldn't change how we load CSS files.

@rxaviers
Collaborator

All files have been renamed by running @jzaefferer's script 026f117, and all UMD wrappers have been updated accordingly. 2eeb5b5

I've tested that the require.js build is working just fine (so, Download Builder supports it), and also that the resulting bundle has no jshint issue: https://gist.github.com/rxaviers/7757969

It needs more eyeballs to make sure the rename did not mess up demos, or tests. But, I guess this has been tested already.

@jzaefferer jzaefferer commented on the diff
ui/autocomplete.js
((11 lines not shown))
-(function( $, undefined ) {
+(function( factory ) {
+ if ( typeof define === "function" && define.amd ) {
+ // AMD. Register as an anonymous module.
+ define([
+ "jquery",
+ "./core",
+ "./widget",
+ "./position",
+ "./menu"
+ ], factory );
+ } else {
+ // Browser globals
+ factory( jQuery );
+ }
+}(function( $ ) {
$.widget( "ui.autocomplete", {
@jzaefferer Owner

Applies to all widgets, just picking a random one: We add a return in front of the $.widget call, otherwise c0ab710 (for http://bugs.jqueryui.com/ticket/9467 ) isn't useful for our widgets.

@rxaviers Collaborator
rxaviers added a note

Yeap, now that widgets return its constructors, we are able to. Will add a fix.

@rxaviers Collaborator
rxaviers added a note

Fixed by d21eb1b and c6ef691

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer
Owner

Apart from adding the return statement as mentioned in my previous comment, we should add some way to test the define calls. Its bad to add all the UMD wrappers without ever executing one of the two paths ourselves.

@jzaefferer
Owner

Discussed the return statements with Rafael on IRC: Since several widgets run more code after the initial $.widget call, he'll add return $.ui.autocomplete; at the end, accordingly for each widget.

@rxaviers
Collaborator

Please, review d21eb1b and c6ef691, specially datepicker, effect, effects (eg. effect-bounce), and widget.

@tjvantoll
Collaborator
@rxaviers
Collaborator

We should really use return $.widget( ... ) where possible, which should be most widgets.

590e0ea

@rxaviers
Collaborator

Updated description with "Update or replace resource loader with an AMD loader. If that turns out to be a bad idea, add a test for each widget that loads it via AMD" TODO item.

@rxaviers
Collaborator

About an item on description:

Bring back support for testing minified files in unit tests. Needs to use actual build output.

Should we revert and include back the build task? Am I missing anything here?

@scottgonzalez

Should we revert and include back the build task? Am I missing anything here?

Nah, we'll figure something out later. This shouldn't hold up the AMD changes. It was just a nice feature that went away when we removed the build task, but we were very rarely testing the minified files anyway.

@rxaviers
Collaborator

I've drafted the "update or replace resource loader with an AMD loader" in two different ways for comparison:

  • Minimal change;
  • AMDify tests;

Minimal change
https://github.com/rxaviers/jquery-ui/commits/UMD-amdify-tests-minimal

Changes have been done in a very minimal way to allow loading resources with AMD. Only the tests ./tests/unit/accordion/accordion.html have been updated so far. All other tests need to be updated as well, but very few more changes seems needed. Note that jshint test (see comment with FIXME message) can't run for some reason (I guess due to $.get); also the css styles were supposed to be loaded after fetched with $.get, this isn't working for me.

AMDify tests
https://github.com/rxaviers/jquery-ui/commits/UMD-amdify-tests

The whole test suite has been AMDified. Only the tests ./tests/unit/accordion/accordion.html have been updated so far. All other tests need to be updated as well.

Which one?

@tjvantoll
Collaborator

I'm a fan of using AMD in the tests files themselves. IMO since we have the dependencies specified in the source files now we might as well use them.

@mikesherov
Collaborator

I'm a fan of using AMD in the tests files themselves. IMO since we have the dependencies specified in the source files now we might as well use them.

:+1:

@jzaefferer
Owner

Looking at the two options Rafael put together, the UMD-amdify-tests branch looks much better, but also leaves a lot to be desired. Since this PR works as-is, I suggest we land this, then start a new PR to work on AMDifying the unit tests.

@tjvantoll
Collaborator

:+1:

@tjvantoll
Collaborator

I took some time to look over the branch today and I noticed a few things:

  • The CSS files fail to load in all tests because the "ui." has not been removed from the loadResources() call. I'll push a commit to the branch to fix this later today.
  • Several tests fail in the branch that do not fail in master. I'm not exactly sure why yet; I'll try to dig into this more.
  • master fails to merge cleanly because of jquery.ui.base.css. It appears that file, and that file alone, was not renamed properly by @jzaefferer's script.
@tjvantoll
Collaborator

The test failures were a result of the CSS files not being imported correctly. I fixed that in 1034f7a.

I also updated the datepicker demos to handle the "Copy updated i18n files to datepicker demos" checkbox on this PR.

I tested out the files in a small app with require.js and everything worked as expected. The tests and demos both run fine and the code looks good.

We might want to address why jquery.ui.base.css didn't get renamed properly, but other than that I say this is good to land.

@rxaviers
Collaborator

Master has updates for jquery.ui.base.css on 28310ff. Although, I don't know why merge (or rebase) doesn't resolve this automatically.

@rxaviers
Collaborator

By the way, thanks for the review and fixes.

@rxaviers
Collaborator

Rebased.

PS: Just figured out why git didn't revolve the rename automatically. Because, it's not a simple rename, it's a rename plus content changes (from @import url("jquery.ui.<widget>.css"); to @import url("<widget>.css");)

@tjvantoll
Collaborator

Ah, that makes sense.

@scottgonzalez

We really need to expose $.Widget in addition to $.widget. I'm ok with waiting until this lands in master though, since I want to split some other files as well.

@rxaviers
Collaborator

Split core into multiple files.

Is this going to happen before or after 1.11?

@scottgonzalez

Split core into multiple files.

Is this going to happen before or after 1.11?

I'd like to do it before 1.11.

@rxaviers
Collaborator

I'd like to do it before 1.11.

Ok, thanks. I will add one issue to handle that on DB.

@rxaviers
Collaborator

Changes, per @scottgonzalez review, complete.

@rxaviers rxaviers referenced this pull request from a commit
@rxaviers rxaviers Wrap source files with UMD return exports
Fixes #9464
Ref #1029
a7a89a7
@rxaviers rxaviers referenced this pull request from a commit
@rxaviers rxaviers Wrap I18n files with UMD return exports
Fixes #9464
Ref #1029
af12171
@rxaviers rxaviers referenced this pull request from a commit
@rxaviers rxaviers Remove "Depends" comment
Fixes #9464
Ref #1029
a2e034b
@rxaviers rxaviers referenced this pull request from a commit
@rxaviers rxaviers All: Remove "Depends" comment 8bef924
@rxaviers rxaviers closed this in 21154cf
@rxaviers rxaviers deleted the UMD branch
@rxaviers rxaviers referenced this pull request from a commit
@rxaviers rxaviers I18n: Rename all files, removing the "jquery.ui." prefix
Amend fix for 21154cf;

Ref #9464
Ref gh-1029
086dad6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.