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

express server support #872

Merged
merged 1 commit into from
Aug 17, 2016
Merged

express server support #872

merged 1 commit into from
Aug 17, 2016

Conversation

prasunsultania
Copy link
Contributor

supporting express server so, that it is easier to use node-soap in an existing express server without affecting other routes.

It supports middleware for body parsing like body-parser as well.

Please see the tests included in commit.

@coveralls
Copy link

coveralls commented Aug 12, 2016

Coverage Status

Coverage increased (+0.2%) to 92.696% when pulling c4490dd on prasunsultania:master into 59aa639 on vpulim:master.

@jsdevel
Copy link
Collaborator

jsdevel commented Aug 12, 2016

Looks good to me. I'd like @herom to weigh in as well.

@prasunsultania
Copy link
Contributor Author

@jsdevel / @herom Any update on this ? We want to use this feature in our ongoing project.

if (!self.authorizeConnection(req)) {
res.end();
return;
if(typeof server.route === 'function' && typeof server.use === 'function'){
Copy link
Contributor

Choose a reason for hiding this comment

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

is there any other possibility to check if the passed in server instance is an express app or is this check reliable from at least 4.x upwards?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

instanceof wont work in this case. This will work 4x onwards as use and route are most commonly used express methods.

Current check gives us an another advantage - we have express as dev dependency only :)

@coveralls
Copy link

coveralls commented Aug 17, 2016

Coverage Status

Coverage increased (+0.2%) to 92.692% when pulling 952c7a0 on prasunsultania:master into 59aa639 on vpulim:master.

@prasunsultania
Copy link
Contributor Author

@herom have made recommended changes. Please review.

@herom
Copy link
Contributor

herom commented Aug 17, 2016

Thanks a ton @prasunsultania - LGTM 👍

One last thing: could you please open a PR and update our README.md so that we have it documented, that soap is able to handle an express server instance as well? 😸

@herom herom merged commit eb658b2 into vpulim:master Aug 17, 2016
@prasunsultania
Copy link
Contributor Author

I will open a separate PR after updating docs.

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