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

add ts-ignore to generated type definitions for multisaml strategy #508

Merged
merged 2 commits into from
Dec 15, 2020

Conversation

gugu
Copy link
Contributor

@gugu gugu commented Dec 4, 2020

Description

  1. Currently, we are forced to use ts-expect-error for multi-saml configuration because it extends the method in the base class with a completely different signature. But this ts-expect-error was not propagated to typescript definitions files. This PR makes things even worse - it uses undocumented feature of TS to propagate ts-expect-error in the definitions
  2. to be able to use passport-saml from github I've changed tsc command from prepublishonly to prepack step. In this case tsc will run when package is installed directly from github

In v3 I suggest to change generateServiceProviderMetadata in multisaml strategy to generateServiceProviderMetadataAsync and add a stub with error message fro generateServiceProviderMetadata

Checklist:

  • Issue Addressed: [ ]
  • Link to SAML spec: [ ]
  • Tests included? [ ]
  • Documentation updated? [ ]

Copy link
Collaborator

@cjbarth cjbarth left a comment

Choose a reason for hiding this comment

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

I really would like to see us getting rid of all the "expected" TS errors. TS has grown to the point where manually saying what types to expect or what errors to expect is probably something that we can avoid.

@cjbarth
Copy link
Collaborator

cjbarth commented Dec 14, 2020

I'm not thrilled about the "v3" idea you've presented, but that can be a discussion for another time.

@gugu
Copy link
Contributor Author

gugu commented Dec 15, 2020

@cjbarth yes, I don't like v3 idea as well and I wish there is any way to solve the problem better way

@gugu gugu merged commit a6b0042 into master Dec 15, 2020
@gugu gugu added the bug label Jan 7, 2021
@cjbarth cjbarth deleted the multisaml-tsignore-d-ts branch February 20, 2021 16:32
@cjbarth cjbarth mentioned this pull request May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants