Clean up types

[cjbarth]

Description

Based on the discussion here, there are too many types being emitted from passport-saml from node-saml. This PR correct that and cleans up some linting errors.

[markstos]

LGTM

[cjbarth]

@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.

[ghusse]

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.

[RopoMen]

I can look this closer on monday.

[cjbarth]

@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.

[ghusse]

Definitively, I prefer the version with *

[RopoMen]

@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]

Codecov Report

Merging #813 (4ef23d9) into master (a14f25a) will decrease coverage by 0.68%.
The diff coverage is 100.00%.

❗ Current head 4ef23d9 differs from pull request most recent head a301150. Consider uploading reports for the commit a301150 to get more accurate results

@@            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              

Impacted Files	Coverage Δ
src/index.ts	100.00% <ø> (ø)
src/types.ts	0.00% <ø> (ø)
src/multiSamlStrategy.ts	78.78% <100.00%> (ø)
src/strategy.ts	58.71% <100.00%> (-0.92%)	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[cjbarth]

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.