Skip to content

added support for koa middleware + some refactoring for the code #111

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

Closed
wants to merge 1 commit into from
Closed

added support for koa middleware + some refactoring for the code #111

wants to merge 1 commit into from

Conversation

NXTaar
Copy link

@NXTaar NXTaar commented Aug 3, 2016

Added support for koa applications (generator based koa v1.2~). By default, it uses the middleware for Express. If there is the koa flag in the options set to true it will use generator based middleware for koa.
Also did some refactoring for better code readability.

@SpaceK33z
Copy link
Member

This would require Node v4+. I'm also wondering that if we want to support Koa, it's maybe better to use a separated file for this. This looks very hacky.

Copy link
Member

@SpaceK33z SpaceK33z left a comment

Choose a reason for hiding this comment

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

There are tests now, that also test if it works with express. If you fix the merge conflict, and add tests with koa, I'll take another look.

@SpaceK33z
Copy link
Member

@NXTaar, you still interested in fixing this?

@NXTaar
Copy link
Author

NXTaar commented Sep 19, 2016

Yes, I am. At the present moment, I'm just too busy at my work's project, so I'll return to this issue, as soon as I get free.

@grawk
Copy link
Contributor

grawk commented Oct 11, 2016

This PR is interesting to me, because it does attempt to refactor the module to be useful in multiple contexts. I'm currently working on a "construx" module for webpack and finding this module as a great starting point (for reference, construx).

Because the main "construx" module furnishes the middleware, it would call out to my construx-webpack module only to access the memory-fs. Therefore, I need to remove the references to req/res/next from this module's code in order to do that.

I haven't thought through it completely yet, but it seems it should be possible to refactor this module such that the essential part of it (pass a file name, get the bundled file content in the callback) can be pulled up into an exportable function or separate module which this module can depend on.

Anyway, just throwing that out there as food for thought.

@SpaceK33z
Copy link
Member

@grawk, I'm open to refactoring this package so that it is easier to add support for other servers (e.g. Koa) or plugins.

I don't have time for this myself currently, so if anyone is willing to take a stab at it: I'll happily review a PR. You could even do this without adding Koa support; that could be done in a second PR.

Note though that there are integration tests now, so it's important to update these, and preferably add more unit tests.

@CrocoDillon
Copy link

Hey! I'm interested to see how this PR progresses and am happy to help if needed.

Perhaps a separate file is more interesting if requiring a higher version of Node than previously for connect users is an issue. Something like import webpackDevMiddleware from 'webpack-dev-middleware/koa' could work.

Here's the thing, Koa 2 will be different. So you'd also need import webpackDevMiddleware from 'webpack-dev-middleware/koa2' or something.

@SpaceK33z
Copy link
Member

@CrocoDillon, I'm closing this PR since it will need a full rewrite. In #140, the express-specific code is separated from the rest of the middleware. This makes it a lot easier to add support for e.g. Koa. If you want to help, I would really like it if you could help me review that PR. That would definitely speed things up.

@SpaceK33z SpaceK33z closed this Oct 21, 2016
@NXTaar
Copy link
Author

NXTaar commented Oct 21, 2016

Hey, PR starter here. 22th i will implement some fixes and improvements according that comments, you guys wrote

@SpaceK33z
Copy link
Member

@NXTaar, awesome. I would suggest to base your PR on the changes in #140.

@NXTaar
Copy link
Author

NXTaar commented Oct 21, 2016

@SpaceK33z ok, i'll take a look

@NXTaar
Copy link
Author

NXTaar commented Oct 22, 2016

@SpaceK33z The #140 is good, i suggest to resolve and merge it first, and then i will make a new PR to merge my koa-supporting changes. I want to avoid to doPR's to @grawk's fork. Better we all improve the original :-)

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.

4 participants