Clean startup #422

Merged
merged 29 commits into from Sep 26, 2012

Projects

None yet

7 participants

@mojit0
mojit0 commented Aug 23, 2012

Unified startup sequence for 'npm start' and 'mojito start' to rely on a common mojito.js file in lib.

The mojito.js file is require()'d by index.js and server.js (as well as build.js, compile.js, and start.js).

The App object formerly in utils.js has been removed and utils.js has been cleaned up.

New unit tests covering a majority of the new API are in the arrow test directory. Obsolete tests and examples which relied on the old API have been removed.

Manually tested 'mojito start', 'npm start', 'node server.js', and 'foreman start' (Heroku).

Also ran 'mojito test', 'mojito test -c', 'mojito compile all', 'mojito build html5app', 'mojito jslint' to be sure they're clean.

@travisbot

This pull request passes (merged 9b77578 into 0492d29).

@drewfish
Member

Without looking in detail at this yet, a comment and a question:

  • This PR seems to also include removal of examples/sandbox/, which should really be a separate commit.
  • Has this new approach been tested by deploying an app to Manhattan?
@drewfish drewfish and 2 others commented on an outdated diff Aug 23, 2012
lib/app/archetypes/app/default/Procfile
@@ -0,0 +1 @@
+web: node server.js
@drewfish
drewfish Aug 23, 2012 Yahoo Inc. member

Hmm... Procfile looks like a Heroku-specific thing. (Unfortunate that it's not called Heroku instead (similar to Travis's travis.yaml).) Some ideas to address this confusion:

  • Add a comment to this file, which explains what it's for.
  • Remove the file entirely, and update our docs to explain how to get a mojito app running on Heroku.
  • Better yet, I think Heroku might work just fine without a Procfile if we update the app's package.json such that node start starts up mojito.
@mojit0
mojit0 Aug 23, 2012

npm start works without any additional changes to the package.json. I'll remove the Procfiles.

@FabianFrank
FabianFrank Aug 23, 2012

I don't thick we should add a Procfile, because as @drewfish said this is Heroku proprietary.

@drewfish drewfish commented on the diff Aug 23, 2012
lib/index.js
*/
-module.exports.include = function(path) {
- require('./' + path);
-};
+module.exports = require('./mojito');
@drewfish
drewfish Aug 23, 2012 Yahoo Inc. member

I'm not against moving index.js to mojito.js, just curious, why is that valuable?

@mojit0
mojit0 Aug 23, 2012

could remove the file entirely, but convention and history mean applications expect it here.

@mridgway
mridgway Aug 23, 2012 collaborator

I'd argue for just have the index.js rather than two files if we're going to follow convention.

@drewfish
drewfish Aug 23, 2012 Yahoo Inc. member

If we do keep index.js, we should put a comment so we remember why we have the indirection. Could be as simple as:

// convention and history mean applications expect there to be an index.js
module.exports = require('./mojito');
@mojit0
mojit0 Aug 23, 2012

We must have index.js for Manhattan, but we don't want it to be full of "real code" or it becomes a mess for every other environment. Hence we have mojito.js with the real logic and both the server.js (which is what npm start uses by default) and the index.js (which is what Manhattan requires) simply point to the real code.

@mojit0
mojit0 Aug 23, 2012

Actually, I could be confusing the Mojito framework index.js with the Mojito application index.js file here. In which case we could potentially remove the index.js file from the framework. Individual applications still need a Manhattan-compatible index.js if they're going to deploy to Manhattan.

@drewfish drewfish and 2 others commented on an outdated diff Aug 23, 2012
@@ -4,10 +4,4 @@
* See the accompanying LICENSE file for terms.
*/
-YUI.add('HelloActionSpeedy', function(Y) {
-
- Y.mojito.actions.speedy = function(ac) {
- ac.done('speedy action!!');
- };
-
-});
+module.exports = require('./lib/mojito').createServer();
@drewfish
drewfish Aug 23, 2012 Yahoo Inc. member

This change seems very very strange. The file has gone from one that registers actions on the controller, to one that creates a mojito app. Yet, it's still in the /actions/ subdirectory of a mojit. I think this is perhaps an error.

(Of course, since it looks like examples/sandbox/ex1/ is doomed, the issue is probably moot.)

@mridgway
mridgway Aug 23, 2012 collaborator

Looks like this was probably just git interpreting a file move due to some similarities in the files (copyright mostly).

@FabianFrank
FabianFrank Aug 23, 2012

This will not work with all hosters, because there is no call to .listen(). A lot of hosters expect you to call listen by yourself. Not calling listen() is the main reason why Mojito doesn't deploy out of the box on hosters and why I submitted pull request #267.

@rwaldura

Silly question: have we verified that, with these changes, a Mojito app still starts in Manhattan?

@mojit0 mojit0 was assigned Aug 23, 2012
@travisbot

This pull request passes (merged 873d5d8 into b39ba67).

@isao
isao commented on 4f7269d Aug 23, 2012

+1

@travisbot

This pull request passes (merged 1666833 into b39ba67).

@mridgway
Collaborator

Looking good. +1 from me. Just to confirm, this has no downstream BC breaks correct?

@FabianFrank

-1

@mojit0 Please look at my inline comment regarding the server.js file. This PR will not fix the issue with Mojito apps not being deployable on hosters out of the box. Users will still have to modify server.js and add the .listen() call manually.

@mridgway
Collaborator

@FabianFrank As far as I can tell, listen is called internally: https://github.com/mojit0/mojito/blob/1666833c4436647046e3bb2fa2bc8812c52722f8/lib/mojito.js#L620

Which hosts are you referring to?

@FabianFrank

I recall nodejitsu required me to call listen(), otherwise the app was not reachable.

Heroku requires you to listen on what they give you as $PORT (environment variable). I don't see how this listen() call inside lib/mojito.js can be easily changed to do this by an end user. I prefer to have this startup sequence exposed in server.js as it is in my PR so that it fulfills 3 targets:

  1. works on Manhattan
  2. works on "most" node hosters
  3. can be changed easily to suit proprietary hoster needs (such as listening on a port they pass to you as a parameter or env variable)
@mridgway
Collaborator

Unfortunately we also have the configuration for port in our application.json as well, so it becomes an issue of precedence. https://github.com/mojit0/mojito/blob/1666833c4436647046e3bb2fa2bc8812c52722f8/lib/mojito.js#L64 covers the cases you talked about, but I see your point on making it easy to customize in the app.

@FabianFrank

@mridgway I don't think supporting all hosters should be an effort inside the start logic/sequence of Mojito. Maybe the next hoster calls it $YOURPORT instead of $PORT, I don't think we can cater to all of them. I'd like to have a plain/standard start sequence that is easily extensible for custom hoster needs like $PORT or a Procfile, etc.

@mojit0
mojit0 commented Aug 24, 2012

@FabianFrank server.js is no longer part of the Mojito framework. It was removed by one of the later commits. The server.js files in Mojito applications invoke app.start(), which calls listen(). As to port, or other configuration vars, the createServer call allows you to pass options to configure the server. You can also manipulate them via the app instance before you call start(). Also, all code that uses port in particular, has been changed to look at process.env.PORT. I tested specifically with Heroku foreman and it works just fine. Mojito starts up, it just runs on port 5000. There's also a unit test included which verifies that if you set process.env.PORT yourself Mojito will use that port. So there's plenty of control to ensure that when it comes to the listen/port-number question this should "just work" for most common cases.

@mojit0
mojit0 commented Aug 24, 2012

@mridgway As to BC, I'm almost certain this could break any application since it is clearly an API change. The old utils.App object is gone, so any application which was depending on that object will break. Additional minor changes to API also exist around the Mojito object and MojitoServer (which is effectively now private). That said, I have no issue with that. I don't think we should use that as a criteria for anything other than how we write our release notes until we reach a point where we're willing to ship a 1.0.0 version.

@mridgway
Collaborator

@mojit0 I don't disagree about the BC stuff. I just wanted to ensure that we document it during release.

I do tend to agree with Fabian though. It should be immediately apparent to a user who opens their server.js how they can set the port number for the mojito app (or feed it in from their host). I'd be fine with documentation inside of the server.js, maybe even a commented out line of code that sets the port number.

@mojit0
mojit0 commented Aug 24, 2012

@mridgway Re commenting the server.js files... I can do that :). I think if we're going to do that tho, it'd be good to get some input on precedence as you pointed out. At this point I believe there are three clearly used values in order: any options passed in, any value for process.env.PORT, and the default port value of 8666. There's at least one other place in the code which also uses appConfig to supply data. And there's the long-standing question of what about the process.argv and using input from the command line. I'd like to take that discussion/solution and put it in a new PR tho.

@drewfish drewfish commented on the diff Aug 29, 2012
lib/app/archetypes/app/default/package.json.hb
@@ -16,9 +16,6 @@
"mojito": "0.4.x"
},
"engines": {
- "node": ">= 0.4.2 < 0.5.0"
- },
- "scripts": {
- "start": "mojito start"
@drewfish
drewfish Aug 29, 2012 Yahoo Inc. member

Why are we removing scripts.start? Is that not used by any hosting environment?

@FabianFrank
FabianFrank Aug 29, 2012

AFAIK nodejitsu uses it.

@mojit0
mojit0 Aug 29, 2012

scripts.start is what you enter if you want npm start to do something other than node server.js. we no longer need that redirection to ensure npm start will start mojito consistently.

@FabianFrank
FabianFrank Aug 29, 2012

Yeah, I changed scripts.start to "server.js" for Nodejitsu deployments.

@drewfish
Member

If this works on all the hosting environments (open-source, plus our internal Manhattan), then +1.

@FabianFrank FabianFrank commented on an outdated diff Aug 29, 2012
lib/app/archetypes/app/default/package.json.hb
@@ -16,9 +16,6 @@
"mojito": "0.4.x"
},
"engines": {
- "node": ">= 0.4.2 < 0.5.0"
- },
- "scripts": {
- "start": "mojito start"
+ "node": ">= 0.6.x"
@FabianFrank
FabianFrank Aug 29, 2012

From https://npmjs.org/doc/json.html#X-Version-Ranges I don't think that ">= 0.6.x" is a valid way to specify a version. It should either be "0.6.x" or ">= 0.6.0". From my experiments with Heroku and Nodejitsu I have seen that "0.6.x" always works but ">= 0.6.0" caused problems, so I would recommend using "0.6.x".

This was referenced Aug 30, 2012
@travisbot

This pull request passes (merged 88da613 into 7558065).

@isao
isao commented Sep 25, 2012

@mojit0 any reason not to land this?
@zhouyaoji something to add to docs when it lands..

@mojit0
mojit0 commented Sep 25, 2012

@isao I think it's ok. Was waiting to ensure tests passed properly and for the current release to be cut so we had time in case some edge case popped up.

@isao
isao commented Sep 25, 2012

perhaps the last commit corresponded to github/travis downtime. 0.4.5 was just cut it would be a good time to merge this. otoh you can merge or rebase latest develop, see if travis tests again.

@mojit0 mojit0 merged commit 23779bb into yahoo:develop Sep 26, 2012

1 check failed

Details default The Travis build failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment