Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Flex all the things #21

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

alexindigo
Copy link
Contributor

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.

@@ -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,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Additionally, it might be nice to do the option rename in a separate PR. Not a huge deal tho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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? :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

(We'll have to bump the major version if we remove devMode anyways)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

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 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).

@goatslacker
Copy link
Collaborator

I'll probably cherry-pick some commits from here and ship them in the 1.x branch

@alexindigo
Copy link
Contributor Author

@goatslacker Yep that was wrong branch. Updated.

@alexindigo
Copy link
Contributor Author

Looks like I missed stuff on my rush to commit. Added pre-commit to prevent such things.

@alexindigo
Copy link
Contributor Author

@goatslacker What are the commits you're not happy with? Thank you.

@alexindigo alexindigo force-pushed the flex_all_the_things branch from 2bd0404 to 29a9021 Compare June 28, 2016 07:48
@@ -7,6 +7,6 @@ describe('Hypernova server', () => {
});

it('starts up the hypernova server without blowing up', () => {
hypernova({ devMode: true, getComponent: () => {} });
hypernova({ clustering: false, getComponent: () => {} });
Copy link
Collaborator

Choose a reason for hiding this comment

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

if this is the default we can leave this off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you ok with me changing defaults? If so, I'd clean things up.

@goatslacker
Copy link
Collaborator

Still looking into this. Haven't forgotten about this PR :)

@alexindigo
Copy link
Contributor Author

👍

goatslacker added a commit that referenced this pull request Jul 18, 2016
@goatslacker
Copy link
Collaborator

Ok so I've merged most of this here 6fd1f90

All that is remaining is the change of devMode -> clustering.

@goatslacker
Copy link
Collaborator

Lets get this merged in. I think all we need is a better name other than clustering.

How about enableCluster? We can set the default to false and document this.

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.

@alexindigo alexindigo force-pushed the flex_all_the_things branch from 29a9021 to e45a76c Compare July 19, 2016 07:36
@alexindigo
Copy link
Contributor Author

@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"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah we don't really need this, tests are run as part of CI here on github so we should be fine without it

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 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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Broken PRs are just a rebase away from being working PRs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll kill the thing. :)

My intention was to automate things, to reduce human workload. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's no more.

@alexindigo
Copy link
Contributor Author

@goatslacker do you mind to release that minor version? Thank you.

@goatslacker
Copy link
Collaborator

I'll release it tomorrow. I have to prep a changelog :)

@alexindigo
Copy link
Contributor Author

@goatslacker Cool, thanks.

@alexindigo
Copy link
Contributor Author

@goatslacker Hey, any news on the release front? :) Thank you.

@goatslacker
Copy link
Collaborator

[email protected] is on npm

@alexindigo
Copy link
Contributor Author

Cool. Thanks a lot!

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

Successfully merging this pull request may close these issues.

3 participants