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

Simple Koa server #1

Merged
merged 6 commits into from
Nov 8, 2017
Merged

Simple Koa server #1

merged 6 commits into from
Nov 8, 2017

Conversation

dolezel
Copy link
Contributor

@dolezel dolezel commented Oct 13, 2017

As mentioned on slack main problem with .mjs files there is no tooling for tests now (at least I can't find one), because you can't mix common js (require) with mjs (import).
Solutions I see:

const router = new Router();

router.get('/', list);
router.get('/:id', get);
Copy link

@branoholy branoholy Oct 17, 2017

Choose a reason for hiding this comment

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

Everything LGTM except the list and get methods. Wouldn't it be better to have separated methods that work directly with data and are not useful for router purposes only?

For example, there could be methods like:

const getUser = ({ id }) => ({ id, name: `user ${id}` });
const getUsers = () => [getUser(1), getUser(2)];

and for simple routes, there could be something like this:

const asRoute = fn => ctx => { ctx.body = fn(ctx.params); };

that would be used as:

router.get('/', asRoute(getUsers));
router.get('/:id', asRoute(getUser));

Of course, more complicated routes could have their own functions (like your list and get):

export default (ctx) => {
  const users = getUsers();
  // ... do something with users
  ctx.body = users;
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Yep, sure, I didn't think much about routes and models so far.
I have a solution which I'm using now, but I'm not much satisfied with it...

@dolezel dolezel merged commit 444ebb2 into master Nov 8, 2017
@dolezel dolezel deleted the mjs branch November 8, 2017 09:15
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