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

Improve the typing of the Strategy class hierarchy. #554

Merged
merged 1 commit into from
Apr 5, 2021

Conversation

forty
Copy link
Contributor

@forty forty commented Mar 15, 2021

Description

The goal of this PR is to improve the typing of the Strategy class hierarchy.

The current expected ts error causes problems with some earlier version of TS.

[watch:ts] node_modules/passport-saml/lib/passport-saml/multiSamlStrategy.d.ts(10,5): error TS2416: Property 'generateServiceProviderMetadata' in type 'MultiSamlStrategy' is not assignable to the same property in base type 'Strategy'.
[watch:ts]   Type '(req: Request, decryptionCert: string, signingCert: string, callback: (err: Error, metadata?: string) => void) => void' is not assignable to type '(decryptionCert: string, signingCert?: string) => string'.

See #540 which seems to be affected by this too.

Also, it does not make any sense to have a method override a method with a different signature, just because they have the same name.

@cjbarth
Copy link
Collaborator

cjbarth commented Mar 16, 2021

This looks good to me. I have a significant refactor of the type system in #548 which addresses some complex issues surrounding requiring a cert. I'm trying to get that landed before changes like this; though we'll certainly get this landed before we release v3. Feel free to have a look at it and offer feedback.

@cjbarth
Copy link
Collaborator

cjbarth commented Mar 18, 2021

Is this something that you wanted fixed in the 2.x branch, or just in master?

@cjbarth cjbarth added the semver-minor This PR requires a semver-minor version bump label Mar 18, 2021
@forty
Copy link
Contributor Author

forty commented Mar 18, 2021

@cjbarth I'm actually mostly interested by the 2.x branch for now, since that's what I'm using at the moment. I'll open a PR for that branch too 👍

@cjbarth
Copy link
Collaborator

cjbarth commented Mar 19, 2021

@forty Would you like to resolve the merge conflicts so we can get this landed as part of the upcoming 3.x release?

@forty forty force-pushed the forty/fix-strategy-typing branch 2 times, most recently from ec5577d to 9835da1 Compare March 22, 2021 10:31
@forty
Copy link
Contributor Author

forty commented Mar 22, 2021

Just rebased on master 👍

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.

As we go along, I'm OK with adding types and making other minor changes in files we're touching to reduce the number of eslint errors.

test/multiSamlStrategy.spec.ts Outdated Show resolved Hide resolved
test/multiSamlStrategy.spec.ts Outdated Show resolved Hide resolved
@forty forty force-pushed the forty/fix-strategy-typing branch from 9835da1 to 87ae2cb Compare March 24, 2021 22:08
@forty forty requested a review from cjbarth March 24, 2021 22:10
@cjbarth cjbarth added this to the 3.0.0 milestone Apr 4, 2021
@cjbarth
Copy link
Collaborator

cjbarth commented Apr 5, 2021

@forty Would you like to get this rebased and we'll get it landed? If you need help, please let me know.

@forty forty force-pushed the forty/fix-strategy-typing branch 2 times, most recently from e37ee06 to 2e10f6d Compare April 5, 2021 19:32
The current expected ts error causes problems with some earlier version of TS.
Also, it does not make any sense to have a method overide a method with a
different signature, just because they have the same name.
@forty forty force-pushed the forty/fix-strategy-typing branch from 2e10f6d to 1911c8e Compare April 5, 2021 19:36
@forty
Copy link
Contributor Author

forty commented Apr 5, 2021

@cjbarth I just rebased on latest master 👍

@cjbarth cjbarth merged commit 4a83196 into node-saml:master Apr 5, 2021
@cjbarth
Copy link
Collaborator

cjbarth commented Apr 5, 2021

If you want this on 2.x, please create a PR there.

@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
semver-minor This PR requires a semver-minor version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants