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

Support AMD loaders that provide a Node.js based server-side component #19

Merged
merged 6 commits into from
Apr 9, 2013

Conversation

rbackhouse
Copy link
Contributor

I have an AMD loader that provides a server-side component (Node.js based). I would like to add the loader to the set of implementations.

To do this I have modified the test framework to support manifest entries providing installers. A manifest can contain the module id of an installer module. e.g

exports.manifest.zazl = {
  name:   'zazl @ 1.0.2',
  impl:   'zazl/zazl.js',
  config: 'zazl/config.js',
  installer: '../impl/zazl/installer'
};

When the test framework server is started it will scan the manifests for installer properties. If any are found the installer module is loaded and an expected install function called.

installer = require(manifest[framework].installer);
// Call the installer with the express app object allowing it to setup any handlers
installer.install(app, installerCallback);

The express "app" object is passed to the installer along with a reference to an installerCallback function the the installer calls to indicate it is complete. The callback function starts the framework once all discovered installers have completed.

If no installers are discovered then the server is started directly.

An additional change in this pull request is to add npm as a dev dependency to the package.json. This allows installers to use npm to programmatically install what they need.

An example installer look like this :

var npm = require('npm');
var path = require('path');
var fs = require('fs');
var util = require('util');

function startZazl(zazloptimizer, app) {
    var testsdir = fs.realpathSync(path.join(__dirname, "../../tests"));
    var optimizer = zazloptimizer.createConnectOptimizer(testsdir, false);
    app.use("/_javascript", optimizer);
}

exports.install = function(app, cb) {
    npm.load([], function(err, npm) {
        if (err) {
            cb("zazl", false);
            return;
        }
        var zazloptimizer;
        try {
            zazloptimizer = require('zazloptimizer');
            util.log("zazloptimizer is already installed");
            startZazl(zazloptimizer, app);
            cb("zazl", true);
        } catch (e) {
            npm.commands.install(['zazloptimizer'], function() {
                util.log("zazloptimizer has been installed");
                zazloptimizer = require('zazloptimizer');
                startZazl(zazloptimizer, app);
                cb("zazl", true);
            });
        }
    });
}

function startZazl(zazloptimizer, app) {
var testsdir = fs.realpathSync(path.join(__dirname, "../../tests"));
var optimizer = zazloptimizer.createConnectOptimizer(testsdir, false);
app.use("/_javascript", optimizer);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a nit. Should we ask installers to put their requested directories into some kind of namespace like /zazl/_javascripts this way other implementors don't accidentally clobber each others express routes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea. I've updated the Zazl impl to include a prefix

@thecodedrift
Copy link
Contributor

Thanks for adding these commits. Just a few questions:

  1. Do we think a sleep value of "20" is okay? Seems like that will really slow down Travis-CI (maybe that's not a big deal)
  2. I did a quick check, but as much of zazl.install() as possible is async, yeah? This way we have a good example for other frameworks that have a server component.

@rbackhouse
Copy link
Contributor Author

  1. I had to bump sleep value up to ensure that npm completed installing everything. With it set to 3 this build https://travis-ci.org/amdjs/amdjs-tests/builds/6046975 failed (even though it reported as successful). 20 might be a bit high. I can experiment with lower values if need be
  2. Not sure what you mean here. Can you elaborate ?

@thecodedrift
Copy link
Contributor

Can disregard. My biggest concern was making sure exports.install = function(app, cb) was async, which it is.

rbackhouse added a commit that referenced this pull request Apr 9, 2013
Support AMD loaders that provide a Node.js based server-side component
@rbackhouse rbackhouse merged commit 82c905b into amdjs:master Apr 9, 2013
@rbackhouse
Copy link
Contributor Author

The build for the merge still hit the timeout issue

https://travis-ci.org/amdjs/amdjs-tests/builds/6180493

but then I triggered another build via the amdjs-tests travis page and it worked fine

https://travis-ci.org/amdjs/amdjs-tests/builds/6182323

Ideally we would want the launch of phantom-server.js to wait for some notification that the server has completed starting.

@jakobo any thoughts ?

@thecodedrift
Copy link
Contributor

I have a few ideas. The most refined is this:

  1. Add a server_wait.js to the server directory. It blocks until localhost:4000 is available
  2. rewrite the .travis.yml line node ./server/phantom-server.js to node ./server/server_wait.js && node ./server/phantom-server.js
  3. remove the sleep statement, as it would be no longer needed. We'd instead block.

Think that'll work? I may have some time to code an example today, but it kind of depends on my afternoon's meetings.

@rbackhouse
Copy link
Contributor Author

Yes, I think that should do the trick. I'll take a stab and put together a new pull request.

@rbackhouse
Copy link
Contributor Author

@jakobo I opened up a new Pull request #20. The test results look good. This looks to be a much better solution. Thanks

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

Successfully merging this pull request may close these issues.

2 participants