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

Upgrade roc-package-web-app to use Koa@2 #30

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

Conversation

pmrukot
Copy link

@pmrukot pmrukot commented Jan 30, 2018

Summary

  • Updated repository to use newest roc-repo version and jest, roc.config.js was adjusted
  • Koa@1 -> Koa@2
  • Updated all packages of roc-package-web-app to be Koa@2 compatible
  • Middlewares were rewritten to be async functions
  • koa-accesslog has been deleted due to incompatibility, now the package uses local implementation (ref src/app/middlewares/accesslog.js)
  • Moved middlewares to src/app/middlewares
  • koa-errors -> koa-error
  • Added some middleware tests

This is a breaking change, middlewares need to be now defined as async functions instead of generators. An API did not change so Im not sure if there are any changes required in packages like: roc-package-web-app-dev, roc-package-web-app-react(-dev). Upgrading those packages to use this one would be also a breaking change.

However, the package should not break the current apps - just throw a warning that generators will be deprecated in Koa v3.

Only tested it with examples/web-app/client-server, however more advanced test cases might be needed.

Had problems with testing createServer since it was breaking Jest and stoped executing test cases.

Need some help with actions required (if there are any regarding other packages) and prerelease process/plan. And review if the changes are sane of course 🙃

BREAKING CHANGE: Middlewares need to be defined as an async funcions instead of generators.
Copy link
Member

@lizell lizell left a comment

Choose a reason for hiding this comment

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

Code looks good. It is of course implied, but maybe add a note that we now require Node 8.

I would also love to have the accesslog middleware behind a config toggle.

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