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

express 5.x. move types to devDependencies #952

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

charlie-harvey
Copy link

Description

Express 5 is out! NestJS 11 is out! It relies on Express 5. But I can't do cool saml things because this library pushes @types/express on me at version 4. So I updated the package.json. That's all.

Checklist:

  • Issue Addressed: [Express 5 ]

@shlomisas
Copy link

Any update on this one?? it also holds up back merge an important PR

Copy link
Contributor

@markstos markstos left a comment

Choose a reason for hiding this comment

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

I'm aligned with this.

It should not be the job of this project to ship the types for Express, Passport or Passport-Strategy.

Also, fine to use Express 5 instead of 4 for testing.

@cjbarth
Copy link
Collaborator

cjbarth commented Mar 11, 2025

This is not an appropriate change because we export types that inherit from other types, so we have to export them for this all to work. As an example see

export abstract class AbstractStrategy extends PassportStrategy {
.

@cjbarth
Copy link
Collaborator

cjbarth commented Mar 11, 2025

We're also not going to take on a semver major bump for a single update. We'll do it with the deprecation of a Node version and other updates to dependencies all around.

@cjbarth
Copy link
Collaborator

cjbarth commented Mar 11, 2025

We are preparing a round of major releases starting with the xml-crypto library. We'll roll that up to node-saml with its semver major release, then we'll do the same here with passport-saml. If this is really blocking you, I suggest temporarily using a fork until this work is completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants