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

Use latest visit API #71

Merged
merged 3 commits into from
Oct 16, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 0 additions & 65 deletions app/initializers/fastboot.js

This file was deleted.

14 changes: 3 additions & 11 deletions app/initializers/ajax.js → app/initializers/server/ajax.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*globals najax, FastBoot, Ember*/
/*globals najax, Ember*/
var nodeAjax = function(url, type, options) {
var adapter = this;

Expand All @@ -22,15 +22,7 @@ export default {
name: 'ajax-service',

initialize: function(application) {
// Detect if we're running in Node. If not, there's nothing to do.
if (typeof document === 'undefined') {
application.register('ajax:node', {
create: function() {
return nodeAjax;
}
});

application.inject('adapter', 'ajax', 'ajax:node');
}
application.register('ajax:node', nodeAjax, { instantiate: false });
application.inject('adapter', 'ajax', 'ajax:node');
}
};
17 changes: 17 additions & 0 deletions app/initializers/server/dom-helper-patches.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/*globals Ember, URL*/
export default {
name: "dom-helper-patches",

initialize: function(App) {
// TODO: remove me
Ember.HTMLBars.DOMHelper.prototype.protocolForURL = function(url) {
var protocol = URL.parse(url).protocol;
return (protocol == null) ? ':' : protocol;
};

// TODO: remove me https://github.com/tildeio/htmlbars/pull/425
Ember.HTMLBars.DOMHelper.prototype.parseHTML = function(html) {
return this.document.createRawHTMLSection(html);
};
}
};
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
/*globals Ember*/

// When using `ember fastboot --serve-assets` the application output will
// already be rendered to the DOM when the actual JavaScript loads. Ember
// does not automatically clear its `rootElement` so this leads to the
Expand All @@ -8,6 +10,8 @@
// application will replace the pre-rendered output

export default {
name: "clear-double-boot",

initialize: function(instance) {
var originalDidCreateRootView = instance.didCreateRootView;

Expand Down
75 changes: 68 additions & 7 deletions index.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,32 @@
/* jshint node: true */
'use strict';

var Funnel = require('broccoli-funnel');

// Expose the an factory for the creating the `Application` object
// with the proper config at a known path, so that the server does
// not have to disover the app's module prefix ("my-app").
//
// The module defined here is prefixed with a `~` to make it less
// likely to collide with user code, since it is not possible to
// define a module with a name like this in the file system.
function fastbootAppModule(prefix) {
return [
"",
"define('~fastboot/app-factory', ['{{MODULE_PREFIX}}/app', '{{MODULE_PREFIX}}/config/environment'], function(App, config) {",
" App = App['default'];",
" config = config['default'];",
"",
" return {",
" 'default': function() {",
" return App.create(config.APP);",
" }",
" };",
"});",
""
].join("\n").replace(/\{\{MODULE_PREFIX\}\}/g, prefix);
}

module.exports = {
name: 'ember-cli-fastboot',

Expand All @@ -11,6 +37,30 @@ module.exports = {
};
},

included: function(app) {
if (process.env.EMBER_CLI_FASTBOOT) {
process.env.EMBER_CLI_FASTBOOT_APP_NAME = app.name;
app.options.autoRun = false;
}

// We serve the index.html from fastboot-dist, so this has to apply to both builds
app.options.storeConfigInMeta = false;
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rwjblue turns out removing this is not as easy as I thought – contrary to the documentation (see "Uses"), these options can't actually be set through the config hook :( (It makes sense, mutating these build options at a random time during build probably has the same problem as setting autoboot during boot 😉)

@stefanpenner can you c/d? If that's true I can (eventually) send a doc PR


config: function(/* environment, appConfig */) {
// do nothing unless running `ember fastboot` command
if (!process.env.EMBER_CLI_FASTBOOT) { return {}; }

return {
EmberENV: {
FEATURES: { 'ember-application-visit': true }
Copy link
Member

Choose a reason for hiding this comment

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

I initially avoided doing this because feature flags were not being merged. Which meant that if you had a feature flag set in the consuming apps config/environment.js it would completely ignore any features added from an addon's config hook.

This may be fixed now with current ember-cli, but I'd love it if you could confirm. The easiest way to test would be to make a throw away app, set some features in it, run ember fastboot and confirm that the values are merged propertly in the resulting files...

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, from what I read, the whole thing is deep merged with lodash. I will confirm!

},
APP: {
autoboot: false
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

both of these works

};
},

contentFor: function(type, config) {
// do nothing unless running `ember fastboot` command
if (!process.env.EMBER_CLI_FASTBOOT) { return; }
Expand All @@ -23,17 +73,28 @@ module.exports = {
return "<!-- EMBER_CLI_FASTBOOT_TITLE -->";
}

if (type === 'vendor-prefix') {
return '// Added from ember-cli-fastboot \n' +
'EmberENV.FEATURES = EmberENV.FEATURES || {};\n' +
'EmberENV.FEATURES["ember-application-visit"] = true;\n';
if (type === 'app-boot') {
return fastbootAppModule(config.modulePrefix);
}
},

included: function() {
treeForApp: function(tree) {
if (process.env.EMBER_CLI_FASTBOOT) {
this.app.options.storeConfigInMeta = false;
process.env.EMBER_CLI_FASTBOOT_APP_NAME = this.app.name;
return new Funnel(tree, {
annotation: 'Funnel: Remove browser-only initializers',
exclude: [
'initializers/browser/*',
'instance-initializers/browser/*'
]
});
} else {
return new Funnel(tree, {
annotation: 'Funnel: Remove server-only initializers',
exclude: [
'initializers/server/*',
'instance-initializers/server/*'
]
});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rwjblue @stefanpenner how does that look? with this I could complete remove the FastBoot global we inject into the sandbox.

By the way, if this works out for us, we might want to move it into the preprocessTree hook so that apps and other addons can use the same pattern

Copy link
Member

Choose a reason for hiding this comment

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

👍 - This looks great to me!


I don't see a way to completely generalize this, but we can chat about that further in a separate issue...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't it just work if we move this code to preprocessTreeFor? Any initializers in with a browser/... prefix will be stripped for server builds and vice versa. Maybe I am understanding it incorrectly – does preprocessTreeFor not get the fully merged app tree with user code and addon code all merged together?

Copy link
Member

Choose a reason for hiding this comment

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

@chancancode - Ya, we could come up with something that could work (I misread initially as postprocessTree not preprocessTree). I'm happy to come up with a specific layout/plan to make it a standard thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll leave that for another time, probably until someone asks for it :P (if you are reading this, you might just want to PR it 😉)

}
};
6 changes: 4 additions & 2 deletions lib/commands/fastboot.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,16 @@ module.exports = {
{ name: 'environment', type: String, default: 'development', aliases: ['e',{'dev' : 'development'}, {'prod' : 'production'}] },
{ name: 'serve-assets', type: Boolean, default: false },
{ name: 'port', type: Number, default: 3000 },
{ name: 'output-path', type: String, default: 'fastboot-dist' }
{ name: 'output-path', type: String, default: 'fastboot-dist' },
{ name: 'assets-path', type: String, default: 'dist' }
],

runCommand: function(appName, options) {
var FastBootServer = require('ember-fastboot-server');
var RSVP = require('rsvp');
var express = require('express');
var outputPath = this.commandOptions.outputPath;
var assetsPath = this.commandOptions.assetsPath;

var server = new FastBootServer({
appFile: findAppFile(outputPath, appName),
Expand All @@ -29,7 +31,7 @@ module.exports = {

if (this.commandOptions.serveAssets) {
app.get('/', server.middleware());
app.use(express.static(outputPath));
app.use(express.static(assetsPath));
}

app.get('/*', server.middleware());
Expand Down
6 changes: 3 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,14 @@
"configPath": "tests/dummy/config"
},
"dependencies": {
"broccoli-funnel": "0.2.8",
"chalk": "^0.5.1",
"contextify": "^0.1.11",
"debug": "^2.1.0",
"ember-fastboot-server": "0.0.2",
"ember-fastboot-server": "chancancode/ember-fastboot-server#new-visit-api",
"express": "^4.8.5",
"glob": "^4.0.5",
"najax": "^0.1.5",
"rsvp": "^3.0.16",
"simple-dom": "^0.2.7"
"rsvp": "^3.0.16"
}
}
8 changes: 6 additions & 2 deletions tests/helpers/start-server.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
var RSVP = require('rsvp');
var path = require('path');
var runCommand = require('ember-cli/tests/helpers/run-command');
var emberCommand = path.join('.', 'node_modules', 'ember-cli', 'bin', 'ember');

module.exports = function runServer(callback, options) {
options = options || { };
Expand All @@ -12,7 +13,7 @@ module.exports = function runServer(callback, options) {
}

var args = [
path.join('.', 'node_modules', 'ember-cli', 'bin', 'ember'),
emberCommand,
'fastboot',
'--port', options.port
];
Expand All @@ -36,7 +37,10 @@ module.exports = function runServer(callback, options) {
args.push(commandOptions);

return new RSVP.Promise(function(resolve, reject) {
runCommand.apply(null, args)
runCommand.call(null, emberCommand, 'build') // build 'dist'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this reflects a real-world problem: you need to now run ember build (probably ember build -prod) before you run ember fastboot --serve-assets, because we need to serve the browser builds from dist. (See the commit message for details)

Unfortunately, it is not easy to refactor the set up to produce both builds in a single command (it relies on a global ENV variable, among other things).

So I just added a new assets-path option for now, but we should revisit this in the future.

.then(function() {
return runCommand.apply(null, args);
})
.then(function() {
throw new Error('The server should not have exited successfully.');
})
Expand Down