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

feat(web-server): allow injection of custom middleware. #1619

Merged

Conversation

shanear
Copy link
Contributor

@shanear shanear commented Oct 12, 2015

Took a crack at a simple way of adding custom middleware.

Closes #1612

@maksimr
Copy link
Contributor

maksimr commented Oct 12, 2015

@shanear Thanks!

Knowledge about connect confuse me.
What do you think about create middleware as launcher or framework?

var DartMiddlewareFactory = function(config) {
return function(request, response, next) {
};
};

module.exports = {
  'middleware:Dart': ['type', DartMiddlewareFactory]
};

example karma.config.js

...
middleware: ["Dart"]
...

P.S. I use name middleware as example.

@shanear
Copy link
Contributor Author

shanear commented Oct 12, 2015

Interesting

It seems from the docs that launchers and frameworks are plugins, and all plugins are npm modules.
http://karma-runner.github.io/0.13/dev/plugins.html

Is that correct? It would be nice to be able to add project specific middleware without having to create a separate npm module for it.

Again this is helpful for working with less modern, enterprise-style api's.

@maksimr
Copy link
Contributor

maksimr commented Oct 13, 2015

It would be nice to be able to add project specific middleware without having to create a separate npm module for it.

@shanear you can inline plugin:

...
plugins: ['karma-*', {'middleware:InlineMiddleware': inlineMiddlewareFactory}]
middliware: ['inlineMiddleware']
...

@shanear
Copy link
Contributor Author

shanear commented Oct 13, 2015

@maksimr great, i'll give that a shot.

@shanear
Copy link
Contributor Author

shanear commented Oct 13, 2015

@maksimr how does this look?

@maksimr
Copy link
Contributor

maksimr commented Oct 13, 2015

@shanear Thanks! Looks good for me 👍

@zzo, @dignifiedquire what do you think about this feature?

@dignifiedquire
Copy link
Member

I think this is a nice addition, but please add some documentation with example and squash your commits into one.

@shanear
Copy link
Contributor Author

shanear commented Oct 14, 2015

@dignifiedquire cool! These docs look ok?

If so, i'll squash everything up.


**Description:** List of names of additional middleware you want the karma server to use.

You must have installed the middleware via a plugin/framework (either inline or via NPM). An example of custom inline middleware is shown below. Additional information can be found in [plugins].
Copy link
Member

Choose a reason for hiding this comment

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

It might be good to mention that this is express/connect middleware, and have a link to their docs on how to write one.

Copy link
Member

Choose a reason for hiding this comment

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

Also it would be good to note that the middleware is used in the order they are included, as the order can matter if I remember correctly.

@shanear
Copy link
Contributor Author

shanear commented Oct 14, 2015

@dignifiedquire i agree with your notes. Updated.

Let me know what you think.

@zzo
Copy link
Contributor

zzo commented Oct 14, 2015

this looks very interesting - middleware authors need to ensure they call 'next' if they don't want to handle a request - def. an advanced feature. What are all of the parameters the factory function gets - more than just 'config'? The karma-dart factory function gets:

function (emitter, /* config.files */ files, /* config.basePath */ basePath,v/* config.karmaDartImports */ customImports, /* config.hostname */ hostname, /* config.port */ port, logger, customFileHandlers, /* config.dart2js */ dart2js) 

@shanear
Copy link
Contributor Author

shanear commented Oct 14, 2015

@zzo Yeah, it's def advanced, but opens up a lot of flexibility in how Karma can be used (IE: we're using it to test our app's integration with a not-very-modern enterprise API). Plus many people working with node seem to be familiar with how connect/express work.

The parameters are provided from the DI system.

@dignifiedquire
Copy link
Member

@shanear the ordering note is still missing, other than that I think it looks good.

@zzo yes if you provide a custom plugin you can specify the parameters you want using di, which should be explained in the plugins docs.

@zzo
Copy link
Contributor

zzo commented Oct 14, 2015

got it thanks - yes I think this is a great feature and ideally the karma-dart (and maybe other plugins?) should be transitioned to to use this ideally :)

@shanear
Copy link
Contributor Author

shanear commented Oct 14, 2015

@dignifiedquire I added a sentence about the ordering.

shanear@8984164#diff-b671a3609847ca11870b94dac48fd028R288
"Middleware will be used in the order listed."

@dignifiedquire
Copy link
Member

@zzo yes it might make a lot of sense to use this in karma-dart from what I remember.
@shanear sorry I can't read, go ahead and squash so we can ship this :)

@dignifiedquire
Copy link
Member

Thanks a lot :octocat:

dignifiedquire added a commit that referenced this pull request Oct 14, 2015
feat(web-server): allow injection of custom middleware.
@dignifiedquire dignifiedquire merged commit 60975de into karma-runner:master Oct 14, 2015
@shanear
Copy link
Contributor Author

shanear commented Oct 14, 2015

awesome 🤘

thanks for all of y'alls help!

@dignifiedquire
Copy link
Member

Published as 0.13.11

@zzo
Copy link
Contributor

zzo commented Oct 14, 2015

mary poppins? I just looked at it the other day and it's up and running from what I could tell

@dignifiedquire
Copy link
Member

@zzo no all good old fashioned hand work

@dignifiedquire
Copy link
Member

@zzo but all automated, just need to run grunt release and I'm done :)

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