Conversation
src/index.ts
Outdated
| } from "./types"; | ||
|
|
||
| export * from "@node-saml/node-saml"; | ||
| export { Profile, SAML, SamlConfig } from "@node-saml/node-saml"; |
There was a problem hiding this comment.
@markstos , as I look at this more, it seems that if a consumer doesn't need it, than node-saml shouldn't export it, so really, we are doing the correct thing by exporting everything someone might need via passport-saml. I came across this when doing node-saml/node-saml#224.
@RopoMen, In your experience, when types reference other types, should those nested types also be exported? I ask because that is what is the case with SamlOption and node-saml/node-saml#220 shows how someone needs those nested types, so I want your perspective before I land this PR.
There was a problem hiding this comment.
As I'm using this new version, it seems that at least types that need to be used by people should be exported. I suppose that every type referenced by options should be exported in order to be sure that people will be able to construct a valid option object without needing to reference node-saml by themselves.
There was a problem hiding this comment.
@ghusse , I completely agree, which is why I've changed passport-saml in this PR to not import from node-saml throughout the code, but, instead, to import from '.', which guarantees that anything we use internally can also be used externally by a consumer. It is these nested types that I have questions about.
I'm thinking that we need to export them since someone might want to use them to type-check the properties they are sending as config; then again, they may not care and just deal with the type warning when compiling TypeScript as the type checker will still inform for improper use of types, they just won't be directly consumable e.g. 'as SomeNodeSamlType` unless they are exported. Thus the conundrum.
There was a problem hiding this comment.
Definitively, I prefer the version with *
There was a problem hiding this comment.
@cjbarth I think passport-saml can be left as it is. When the previous discussion was made I did not look into node-saml "main"/"entrypoint" at all, which is https://github.com/node-saml/node-saml/blob/master/src/index.ts
That is the most important file, it provides the public interface to node-saml. Thereof in passport saml it's ok to export everyting from there. I was afraid of that node-saml was exporting everything, even the internal stuff.
But for type exports, is there any good usage for this MandatorySamlOptions? It is already part of SamlOptions and SamlConfig which I consider main configuration objects.
Codecov Report
@@ Coverage Diff @@
## master #813 +/- ##
==========================================
- Coverage 65.10% 64.42% -0.68%
==========================================
Files 4 4
Lines 149 149
Branches 37 37
==========================================
- Hits 97 96 -1
- Misses 29 30 +1
Partials 23 23
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
I've updated some more of the types here to make the typing much clearer. This could result in a break from some users, so I've added that label and this will have to wait until we're preparing for the 5.x release. |
Description
Based on the discussion here, there are too many types being emitted from
passport-samlfromnode-saml. This PR correct that and cleans up some linting errors.