async/await for saml.ts

[gugu]

To make saml.ts maintainable I've added async/await support for saml.ts

1. Old methods contain just util.callbackify function, which calls async methodNameAsync method
2. Old methods marked as deprecated (do we need to mark them this way?)

Pros:

• less useless code (no checks for errors, then, catch, helper functions to deal with promises and zlib.inflate). -95 lines if we don't count deprecated callback functions (-30 if we count them)
• the code left is written vertically, not diagonally
• better stack traces
  Cons:
• too much changed for a single PR

[markstos]

@gugu Thanks for this work.

@cjbarth @gugu What do think about supporting both promises and callbacks vs the more radical approach of dropping callback support and requiring users uses to use calllbackify if they need callback versions of our methods?

I suspect most of our new users who haven't used passport-saml before would either prefer an async/await interface or we should encourage that direction anyway.

For those with legacy callback codebases, those developers are probably already introducing other modules with promise-only APIs should be familiar with adding callbackify() if needed.

This PR adds some duplicate functions with the Async suffix. We could instead not have these duplicate functions and convert the existing functions to promise APIs.

[gugu]

I thought about this. The problem with keeping the existing name is behavior for users who update to a new major release - callback will never be called. Currently, I marked these functions as @deprecated. All IDE and typescript will mark this code as deprecated. Also, I can add a deprecation warning.

But any decision is fine for me

[cjbarth]

I don't have time to review this right now, but I think it is important that we keep a distinction between exported and internal functions. I'm fine with all internal functions being async/await and only returning promises. External functions, particularly those that match the Passport specification, should support the "node-back" pattern of the last argument being a callback. If no callback is provided, then we can default to returning a promise; any function that returns a Promise can be awaited in client code. In that case, we wouldn't rename the function to have an Async suffix.

Example. Please keep in mind that top-level async/await doesn't work, but the idea is sound. Please let me know if you have questions.

[markstos]

I was about to say about the same thing as @cjbarth. I reviewed our README, and I don't see any functions from "saml.ts" being currently documented as public functions. I'm not concerted about the minority of people who are using an internal, undocumented module directly.

@cjbarth is right that parts of the passport spec involves some callbacks and we need to continue to support those, but I think those are more on the "Strategy" class and not the internal "saml" class.

One of the goals of this project has been to expose a high quality SAML library that doesn't depend on passport and saml.ts is shaping up to be that. I think having it only support async APIs would be cleaner.

[cjbarth]

What objection is there to supporting both callbacks and promises using the pattern I showed? It is a very simple pattern and doesn't presume on a pattern of coding or external transform. The pattern would only have to be for exported functions, and we have very few of those, even if saml.js is a stand-alone library.

[markstos]

@cjbarth I'm OK with supporting both APIs, but it's not my preference.

I know some other popular libraries like Mongoose support both Promise API and callback APIs. In the Mongoose case, they were supporting backwards compatibility. In our case, we are talking about potentially formally exposing an internal module for the first time. An argument in favor of "promises only" is that it's easy to add features than take them away.

A second benefit of "promises-only" is that it simplifies the docs.

Third, the example "node back" code doesn't address exceptions (unless I missed something). Wouldn't we also need to try/catch code, then convert any errors into errbacks? Something like Bluebird's asCallback() uses a similar pattern, but also handles exceptions. http://bluebirdjs.com/docs/api/ascallback.html

One option is to start with a promises-only API and then if there's a demand for callback interface, a pattern like the one you showed can easily support that. If a callback interface is a high-demand feature, it will be easier to add then to triage a stream of issues requesting it.

[cjbarth]

One option is to start with a promises-only API and then if there's a demand for callback interface, a pattern like the one you showed can easily support that. If a callback interface is a high-demand feature, it will be easier to add then to triage a stream of issues requesting it.

I'm all for this idea. I have no immediate use for this version of the library, but no projects that I'm actively working on make use of Promises as first-class citizens (they were started before good native support for Promises), so that is why I'm interested in a way forward. I also would rather not recommend a 3rd party library for promises; even Bluebird says to prefer native promises now.

One question though, do we know how many exported function's we're talking about? I was under the impression it would only be a small handful and those would mostly be API-only functions that dispatch to internal implementations so that we never actually expose the internal functions and can refactor without messing with the API, like the following. In that case, initially adding support for both patterns would be trivial.

function myPublicFunction(options: {}) {
  // validate options?
  return myPrivateFunction(options);
}

function myPrivateFunction(options: {}) {
  // do stuff
  return stuff;
}

As for the error handing, it really won't change between the two patterns. At the deepest level of code, the nodeBack function, you'd have to catch the error with a try/catch in either case so you know to reject the promise or "errback". At the API-function level (myPublicFunction()), it would be the internal .then would call the "nodeback" and .catch call the "errback", trivial boilerplate.

[markstos]

@cjbarth I reviewed the Passport docs some yesterday I see only that the strategy is part of the spec, Our saml.ts is an internal implementation detail as far as that's concerned. From reviewing our own README, we don't document exposing it there either.

What is true is that there have been multiple requests over the years for people to use our SAML functions without Passport. So exposing and documenting the functions in saml.ts as public functions would serve that new purpose without backwards compatibility concerns.

Potentially, this could be split into it's own NPM module that would be a dependency of the passport-saml project. Most of the issues and activity would be happening in the new module. This "node-saml" organization was set up partly with this in mind, as the name is generic and we can easily add a second repo or move to a mono-repo design. There have been some aborted efforts to head in that direction in the past.

[cjbarth]

I completely agree that saml.ts is our internal code and I'm all for opening it up so that others can make use of it. However, I hesitate to just open all of it and let people go nuts. I feel like that could lead to a maintenance headache down the road. I'd like to split off saml.ts into its own thing and just export the functions in saml.ts that the Passport layer (strategy.ts) needs, preferably as shim functions like I mentioned above and refactor the actual "do work" code to another internal-only function. Then we can import saml.ts to the passport package and see what requests for other functions we get regarding saml.ts.

Does that make sense?

[gugu]

What is the decision? I'm okay with any choice