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

Add support for a wrapper array in context #52

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

PlasmaPower
Copy link
Contributor

This is a new way to implement wrappers (another one was #51).

@PlasmaPower PlasmaPower force-pushed the wrappers branch 2 times, most recently from 8fdfefc to ddb4907 Compare March 24, 2016 17:03
@PlasmaPower
Copy link
Contributor Author

Tests should be passing now, had some linting problems.

index.js Outdated
try {
if (fn && context.wrappers) {
context.wrappers.forEach((wrapper) => { fn = wrapper(fn) })
Copy link
Contributor

Choose a reason for hiding this comment

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

Not that it matters, but I find wrapper => { ... } without ()'s around single argument fat-arrows a bit more pleasant.

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 know, I would like that too, and I wouldn't like the brackets either, but standard requires both of those.

@fl0w
Copy link
Contributor

fl0w commented Mar 24, 2016

It sure is less invasive compared to #51, even though it too could be iterable. There's some risk of overuse in user land I guess.

@PlasmaPower
Copy link
Contributor Author

If this merges (with the koa PR) then I'll make a PR for koa-mount, and make a PR to revert the deprecated generators in koa and add wrapper support for koa-convert.

@fl0w
Copy link
Contributor

fl0w commented Mar 24, 2016

It's a little weird to have dependency from within context in compose though, isn't it?

@PlasmaPower
Copy link
Contributor Author

I guess, but I'm not sure that has much of a practical implication. I've already checked that context.wrappers exists, so it shouldn't matter if the context is not standard.

@PlasmaPower PlasmaPower force-pushed the wrappers branch 2 times, most recently from 9292019 to 371960b Compare March 24, 2016 19:34
@PlasmaPower
Copy link
Contributor Author

The code now throws an error if it doesn't end up with a function (or it ends up with a generator function). I think now that we have wrappers, it makes sense for that to be here. However, this makes the code a bit long. Should we abstract lines 43-52 into it's own module (e.g. koa-call-middleware)? This would also solve a problem I had earlier where mount didn't warn me that I had passed it generator middleware by accident, which broke my code.

@PlasmaPower
Copy link
Contributor Author

We could also create context.callMiddleware(middleware, context, next). Edit: That further cements the context dependency though.

@PlasmaPower
Copy link
Contributor Author

Okay, I've consolidated the code into another module: https://github.com/PlasmaPower/call-middleware I haven't published the module to NPM yet, and the module references itself as if it was part of koajs. That's why the build is failing. My plan is someone can clone that repo, push it to a koajs repo (forking it removes the issues tab), make any needed changes, publish the module, then the build will pass and this can be merged. I've already checked that with the module there the build passes.

@jonathanong
Copy link
Member

is this taking .wrappers from the context? if so, -1

@PlasmaPower
Copy link
Contributor Author

Yep. It helps preserve the wrappers and ensure the compatibility, but it also makes compose dependent on context to some extent. The alternative is another paramater, like @fl0w implemented. I prefer the former because it's better for the user, but the latter is less opinionated.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@PlasmaPower
Copy link
Contributor Author

Rebased. Remember tests are failing because they rely on koa-call-middleware which isn't published yet.

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.

None yet

3 participants