Skip to content

AMD support #156

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

Closed
wants to merge 3 commits into from
Closed

AMD support #156

wants to merge 3 commits into from

Conversation

rxaviers
Copy link
Member

No description provided.

@@ -22,23 +20,47 @@ function Builder( jqueryUi, components, options ) {
}

Builder.prototype = {
build: function() {
build: function( callback ) {
var cacheKey = this.jqueryUi.pkg.version + JSON.stringify( this.expandComponents( this.components ) ),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sidenotes:

Builder#build was synchronous (the bundles were created by concatenating strings). Now, with requirejs, it gets async.

Initially, I've tried to avoid changing the workflow of Builder to be asynchronously. I came up with an idea of wrapping rjs output with an Stream (on rjs completion, it set the readable stream as readable) (it was using https://github.com/isaacs/readable-stream to support node 0.8). The idea worked just fine, because Builder could return an output synchronously, and the archiver (zipper) knew how to handle the asynchronous Stream data. Although, it turned out to be a problem for two reasons: (a) on certain places I needed to process the bundles (but, my own stream wrapper was able to chain those operations), (b) packer copies the same bundle into two different locations, when that happens both files reference the same data, after the first data is read by the zipper, the Stream drains out, when it tries to read the second file, the read gets stuck and thats a complicated problem to solve. I gave up and thought it was easier to change Builder to be async :P.

I had prepared the async change terrain with this previous commit 5c336d2.

@rxaviers
Copy link
Member Author

rxaviers commented Aug 1, 2013

@scottgonzalez @jzaefferer ready for review

@@ -182,7 +149,8 @@ function Builder_1_11_0( build, jqueryUi, components, options ) {
var slug;
// Expand categories's pages and posts
if ( (/\/category\//).test( doc ) ) {
slug = path.basename( doc );
// TODO commit separately, replace is due to basename diff between node 0.8 and newer 0.10. 0.8 leaves trailing slash
slug = path.basename( doc ).replace( /\/$/, "" );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im fixing this...

@rxaviers rxaviers mentioned this pull request Aug 1, 2013
8 tasks
@rxaviers
Copy link
Member Author

rxaviers commented Aug 1, 2013

Highlights:

  • Build process is 3x slower. Processing AMD dependencies (via requirejs) is more complex than concatenating files. In my machine, what previously took 1.28s avg, now takes 3.92s avg (download package build-time after files are loaded in memory).
  • Notice, though, the above slowness does NOT affect cached builds. The built files are cached, so for the common downloads, this slowness won't impact users.

@rxaviers rxaviers mentioned this pull request Nov 25, 2013
2 tasks
@rxaviers rxaviers closed this in b7f2e2d Feb 19, 2014
@rxaviers rxaviers deleted the amd-build branch February 19, 2014 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant