Flex all the things #21
Conversation
| @@ -51,7 +51,7 @@ Next, lets configure the development server. To keep things simple we can put th | |||
| var hypernova = require('hypernova/server'); | |||
|
|
|||
| hypernova({ | |||
| devMode: true, | |||
| clustering: false, | |||
There was a problem hiding this comment.
For this one, we're using external clustering solution (pm2) in production,
so having devMode: true flag would be misleading and confusing for other developers.
There was a problem hiding this comment.
While I think this is a better name, "absence" and "false" should always be equivalent - and we want clustering to default to "true".
Can we think of an alternative name where it makes sense to default to false?
There was a problem hiding this comment.
Additionally, it might be nice to do the option rename in a separate PR. Not a huge deal tho.
There was a problem hiding this comment.
Speaking of :) My take on it, that clustering shouldn't be default option, but rather something you opt-in to.
If it's ran in production, it's ran using all sets of configs and environment variables, which leave no thing to default,
and imposing clustering where default matters - in dev mode will make things more confusing than helpful.
So how about keeping it clustering, making default false and bumping major version? :)
There was a problem hiding this comment.
(We'll have to bump the major version if we remove devMode anyways)
There was a problem hiding this comment.
By default it's recommended that you run Hypernova with clustering enabled. What sort of setup are you running in production and what would you typically expect?
There was a problem hiding this comment.
I mean when you run things in production you don't want to leave it to a chance and usually your config is very detailed, so defaults not that important. In dev people tend just run things as is and see what's going on, and having clustering there would be just confusing. Even your readme starts with devMode: true.
As for us, we're using a pm2 and relying on it's scaling/clustering mechanism. And don't need servers to hog all available cpus, especially if not all of the cores might "available" (e.g. nodejs proxy is running on 4/24 cores).
|
I'll probably cherry-pick some commits from here and ship them in the 1.x branch |
|
@goatslacker Yep that was wrong branch. Updated. |
|
Looks like I missed stuff on my rush to commit. Added |
|
@goatslacker What are the commits you're not happy with? Thank you. |
alexindigo
force-pushed
the
flex_all_the_things
branch
from
2bd0404
to
29a9021
Compare
| @@ -7,6 +7,6 @@ describe('Hypernova server', () => { | |||
| }); | |||
|
|
|||
| it('starts up the hypernova server without blowing up', () => { | |||
| hypernova({ devMode: true, getComponent: () => {} }); | |||
| hypernova({ clustering: false, getComponent: () => {} }); | |||
There was a problem hiding this comment.
if this is the default we can leave this off.
There was a problem hiding this comment.
Are you ok with me changing defaults? If so, I'd clean things up.
|
Still looking into this. Haven't forgotten about this PR :) |
|
👍 |
|
Ok so I've merged most of this here 6fd1f90 All that is remaining is the change of |
|
Lets get this merged in. I think all we need is a better name other than How about If you want to rebase/rename I can merge this in when ready. It'll be a major change so I'll couple it with the other PR that has a breaking change. |
to reduce confusion and make it less opionated.
alexindigo
force-pushed
the
flex_all_the_things
branch
from
29a9021
to
e45a76c
Compare
|
@goatslacker Thanks. I rebased and renamed All The Things :) |
| @@ -14,6 +14,9 @@ | |||
| "cover:check": "istanbul check-coverage && echo code coverage thresholds met, achievement unlocked!", | |||
| "test:quick": "babel-node node_modules/.bin/_mocha -R tap test/init.js test/*-test.js" | |||
| }, | |||
| "pre-commit": [ | |||
| "test" | |||
There was a problem hiding this comment.
i'd really prefer not to run tests before every commit - that will slow down rebasing. can we make this pre-update, or, perhaps add it in a separate PR?
There was a problem hiding this comment.
This particular module only runs as pre-commit - http://npmjs.com/pre-commit
I can split it, although rebase doesn't run pre-commit hooks.
There was a problem hiding this comment.
yeah we don't really need this, tests are run as part of CI here on github so we should be fine without it
There was a problem hiding this comment.
I'll remove it if you think it makes things worst,
it's just I myself committed broken code at first :)
I think it would help to reduce number of broken PRs.
There was a problem hiding this comment.
Broken PRs are just a rebase away from being working PRs
There was a problem hiding this comment.
Ok, I'll kill the thing. :)
My intention was to automate things, to reduce human workload. :)
|
@goatslacker do you mind to release that minor version? Thank you. |
|
I'll release it tomorrow. I have to prep a changelog :) |
|
@goatslacker Cool, thanks. |
|
@goatslacker Hey, any news on the release front? :) Thank you. |
|
[email protected] is on npm |
|
Cool. Thanks a lot! |
Trying to make it more developer friendly.
The main change is to split worker's one big thing into separate methods,
to allow downstream apps have hypernova's functionality on different endpoints and/or namespacing (e.g. components vs containers or different versions for backwards compatibility), while preserving original API.
Please let me know if anything else should be done/improved.
Thank you.