convert generator-mw with deprecation warning #683

fl0w · 2016-03-14T14:58:23Z

As discussed with @PlasmaPower in #533 (from issuecomment-196293199 to issuecomment-196326368)

Enable support for old-style middleware using koa-convert with an added deprecation warning.

Left tests as is for now to illustrate impact on current coverage.

PlasmaPower · 2016-03-14T15:35:55Z

You need to fix the existing generator test here. I would recommend also testing that the middleware handles a request as expected. Other than that, I'd say this is good.

fl0w · 2016-03-14T15:53:11Z

Aye, as noted that in the description (that I haven't updated the tests yet). I wasn't sure if this was a direction you were willing to take this; as we discussed deprecating usage in koa-compose - and I was unsure if util.deprecate wrapping was done to taste. I'm on the run and will try to get to it as soon as possible.

PlasmaPower · 2016-03-14T15:55:43Z

I'm fine with the util.deprecate logging, the one thing is that it won't log the name of the middleware that is using a generator, but hardly anybody names their generators anyway and it should be fairly easy to spot.

fl0w · 2016-03-14T17:23:52Z

I'm trying to figure out how I can catch and/or assert that a util.deprecate output was made. Any pointers?

jonathanong · 2016-03-14T17:36:33Z

if we're okay with including koa-convert (which we weren't the last time i remember discussing it), we should use https://github.com/dougwilson/nodejs-depd because it has a better UX - it tells you where you called the deprecated function

fl0w · 2016-03-14T17:41:18Z

@jonathanong saw same usage from cookie (throws deprecation during tests btw). Updating PR as we speak for depd and to include tests.

fl0w · 2016-03-14T19:08:15Z

@tejasmanohar I reverted to "old style" in master (one line, w/ body). Figured there's a reason that specific line exceeds ~72 character limit.

fl0w · 2016-03-14T19:11:36Z

Oops, there seems to be some linting issues. I assumed linting occurred while doing npm run test. One second.

PlasmaPower · 2016-03-15T00:39:53Z

lib/application.js

@@ -100,7 +104,7 @@ module.exports = class Application extends Emitter {

  use(fn) {
    if (typeof fn !== 'function') throw new TypeError('middleware must be a function!');
-    if (isGeneratorFunction(fn)) throw new TypeError('Support for generators has been removed. See the documentation for examples of how to convert old middleware https://github.com/koajs/koa/tree/v2.x#old-signature-middleware-v1x');
+    if (isGeneratorFunction(fn)) { fn = convert(fn); deprecate('Support for generators will been removed. See the documentation for examples of how to convert old middleware https://github.com/koajs/koa/tree/v2.x#old-signature-middleware-v1x'); }


You should probably split this up into four lines now that it isn't a single statement.

Also, we might want to change that link/not link to it since it just says to use koa-convert.

+1 to both ^

tejasmanohar · 2016-03-15T00:57:26Z

What's the end goal? I'd prefer we don't take the responsibility of koa-convert in Koa itself.

PlasmaPower · 2016-03-15T01:15:11Z

@tejasmanohar to make the new koa less of a breaking change. It would follow the Ember and React style where you deprecate something in v+1 and remove it in v+2. In addition, we wouldn't be taking responsibility, it would be less of a breaking change, this wouldn't be guaranteed to not break things. As for taking responsibility of the core of koa-convert, it's not that bad, it's pretty much just co.call wrapped in a function.

tejasmanohar · 2016-03-15T04:15:44Z

@PlasmaPower I understand, but since most but not everything will work out-of-the-box with koa-convert (example), we're bound to receive lots of issues about why X works in Koa v1 but not v2. A clear differentiation between what signature and return values v1 and v2 accepts makes things a lot simpler to wrap your head around and support. Anyways, that's just my take.

fl0w · 2016-03-15T09:15:45Z

Updated deprecation message accordingly.

@tejasmanohar I understand that concern; I am new so my input isn't necessarily valid. I drew inspiration from Ember. This PR arose from the bikeshedding in #533 regarding pro's and con's for releasing v2 before async/await is natively implemented. What this "solves" is being able to release v2 as a stepping stone to an eventual v3 which would release in sync with a native async/await solution.

People want/use v2 already (with and without a build process). There's numerous examples where people already use v2 and do not understand how to use generator middleware in v2 (example). However, this might just be a documentation thing - which I'm willing to take a stab at as well if allowed.

I made this PR to illustrate a potential solution as discussed in #533, which actually was about introducing a deprecation message in koa-convert or koa-compose. I figured I'd alleviate @PlasmaPower from this suggestion while you Koa.js hackers make a decision (if at all it's on the table).

jonathanong · 2016-03-15T19:35:44Z

@koajs yes or no? i don't mind adding this for v2 and removing it in v3.

lourd · 2016-03-15T21:09:07Z

Correct me if I'm wrong, but if this is added in then there wouldn't be any breaking changes, right? You could publish the current v2 branch as a new minor version (1.3), and then 2.x will be the one that removes deprecation warnings.

I'm not one for strict semver doctrine, but I think that makes more sense than bumping the major version twice for one breaking change.

PlasmaPower · 2016-03-15T21:11:38Z

@lourd The flushing headers support breaks an edge case, and deprecating generators means deprecating the entire codebase of v1.

dead-horse · 2016-03-16T02:35:42Z

+1 for add generator function support in v2.

fl0w · 2016-03-16T09:13:25Z

Two questions

Do you want me to add an entry about this to README.md in this PR or should that be done in a separate PR?
The depd message; is it sufficient or should we add a more severe warning e.g. "you milage may vary, strongly recommend v2 middleware"

PlasmaPower · 2016-03-17T21:33:20Z

@fl0w A small note about this in the README might be helpful, maybe modify the note about koa-convert to include this. As for the depd message, I don't like the proposed addition given that it implies that using generators is the problem. Maybe add on "Some v1 middleware may not work with v2, use at your own risk" or something like that.

PlasmaPower · 2016-03-17T21:35:40Z

Somehow this branch is out of date from v2.x, you should probably rebase.

PlasmaPower · 2016-03-18T19:22:44Z

A problem with this is that then generators would work with app.use, but not router.use or app.use(mount(. Should it be implemented in other places using middleware too? That might get a bit messy, but it's nicely wrapped up in koa-convert.

jonathanong · 2016-03-18T22:57:31Z

yeah, docs in the readme are always helpful.

for depd, as long as you say something along the lines of, "generator usage is deprecated in v2.x and will be removed in v3.x. please use the new koa v2 signature w/ async functions".

jonathanong · 2016-03-18T22:58:16Z

lib/application.js

@@ -100,7 +104,11 @@ module.exports = class Application extends Emitter {

  use(fn) {
    if (typeof fn !== 'function') throw new TypeError('middleware must be a function!');
-    if (isGeneratorFunction(fn)) throw new TypeError('Support for generators has been removed. See the documentation for examples of how to convert old middleware https://github.com/koajs/koa/tree/v2.x#old-signature-middleware-v1x');
+    if (isGeneratorFunction(fn)) {
+      deprecate('Support for generators will been removed. ' +


let's add that it'll be removed in v3 and have a link to the docs

jonathanong · 2016-03-18T22:59:04Z

A problem with this is that then generators would work with app.use, but not router.use or app.use(mount(. Should it be implemented in other places using middleware too? That might get a bit messy, but it's nicely wrapped up in koa-convert.

yeah, that'll have to be done in those middleware

otherwise, LGTM!

fl0w · 2016-03-19T08:09:15Z

Ok - give me a few hours (on the run) and I'll:

add a paragraph to README
update depd message
rebase

fl0w · 2016-03-20T10:50:59Z

I would prefer #686 merged (or denied) before this so I can update accordingly.

jonathanong · 2016-03-21T19:23:12Z

@fl0w #686 has been merged!

fl0w · 2016-03-22T06:38:36Z

@jonathanong updated.

I apologise btw. I could have sworn that PR introduced markdown linting which I wanted to conform with.

PlasmaPower · 2016-03-22T14:11:35Z

@fl0w it did, but the linter would force us to make some style changes that we didn't want so we disabled it.

convert generator-mw with deprecation warning

PlasmaPower reviewed Mar 15, 2016
View reviewed changes

jonathanong mentioned this pull request Mar 15, 2016

Koa v2.x #665

Merged

16 tasks

jonathanong modified the milestone: v2.0.0 Mar 15, 2016

jonathanong self-assigned this Mar 17, 2016

jonathanong reviewed Mar 18, 2016
View reviewed changes

Convert generator-mw with deprecation warning

0ac4ff0

jonathanong added a commit that referenced this pull request Mar 22, 2016

Merge pull request #683 from fl0w/v2.x

daf688b

convert generator-mw with deprecation warning

jonathanong merged commit daf688b into koajs:v2.x Mar 22, 2016

fl0w mentioned this pull request Apr 21, 2016

Support Koa v2 koajs/jwt#34

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

convert generator-mw with deprecation warning #683

convert generator-mw with deprecation warning #683

fl0w commented Mar 14, 2016

PlasmaPower commented Mar 14, 2016

fl0w commented Mar 14, 2016

PlasmaPower commented Mar 14, 2016

fl0w commented Mar 14, 2016

jonathanong commented Mar 14, 2016

fl0w commented Mar 14, 2016

fl0w commented Mar 14, 2016

fl0w commented Mar 14, 2016

PlasmaPower Mar 15, 2016

PlasmaPower Mar 15, 2016

tejasmanohar Mar 15, 2016

tejasmanohar commented Mar 15, 2016

PlasmaPower commented Mar 15, 2016

tejasmanohar commented Mar 15, 2016

fl0w commented Mar 15, 2016

jonathanong commented Mar 15, 2016

lourd commented Mar 15, 2016

PlasmaPower commented Mar 15, 2016

dead-horse commented Mar 16, 2016

fl0w commented Mar 16, 2016

PlasmaPower commented Mar 17, 2016

PlasmaPower commented Mar 17, 2016

PlasmaPower commented Mar 18, 2016

jonathanong commented Mar 18, 2016

jonathanong Mar 18, 2016

jonathanong commented Mar 18, 2016

fl0w commented Mar 19, 2016

fl0w commented Mar 20, 2016

jonathanong commented Mar 21, 2016

fl0w commented Mar 22, 2016

PlasmaPower commented Mar 22, 2016

convert generator-mw with deprecation warning #683

convert generator-mw with deprecation warning #683

Conversation

fl0w commented Mar 14, 2016

PlasmaPower commented Mar 14, 2016

fl0w commented Mar 14, 2016

PlasmaPower commented Mar 14, 2016

fl0w commented Mar 14, 2016

jonathanong commented Mar 14, 2016

fl0w commented Mar 14, 2016

fl0w commented Mar 14, 2016

fl0w commented Mar 14, 2016

PlasmaPower Mar 15, 2016

Choose a reason for hiding this comment

PlasmaPower Mar 15, 2016

Choose a reason for hiding this comment

tejasmanohar Mar 15, 2016

Choose a reason for hiding this comment

tejasmanohar commented Mar 15, 2016

PlasmaPower commented Mar 15, 2016

tejasmanohar commented Mar 15, 2016

fl0w commented Mar 15, 2016

jonathanong commented Mar 15, 2016

lourd commented Mar 15, 2016

PlasmaPower commented Mar 15, 2016

dead-horse commented Mar 16, 2016

fl0w commented Mar 16, 2016

PlasmaPower commented Mar 17, 2016

PlasmaPower commented Mar 17, 2016

PlasmaPower commented Mar 18, 2016

jonathanong commented Mar 18, 2016

jonathanong Mar 18, 2016

Choose a reason for hiding this comment

jonathanong commented Mar 18, 2016

fl0w commented Mar 19, 2016

fl0w commented Mar 20, 2016

jonathanong commented Mar 21, 2016

fl0w commented Mar 22, 2016

PlasmaPower commented Mar 22, 2016