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

Update readme on using multiSamlStrategy #531

Merged
merged 3 commits into from Feb 7, 2021
Merged

Conversation

gugu
Copy link
Contributor

@gugu gugu commented Feb 4, 2021

passport-saml/multiSamlStrategy.js is something I'd like to remove:

  1. it contains nothing, just import
  2. it is outside of the source root and tsc/other tools don't include this file
  3. old import depends on internal structure of the project
  4. it uses module-level export

For now let's keep the file for compatibility, but let's update README so new people will not use the old way

passport-saml/multiSamlStrategy.js is something I'd like to remove:
1. it contains nothing, just import
2. it is outside of the source root and tsc/other tools don't include this file
3. old import depends on internal structure of the project
4. it uses module-level export

For now let's keep the file for compatibility, but let's update README so new people will not use the old way
@cjbarth
Copy link
Collaborator

cjbarth commented Feb 4, 2021

I feel like we can remove that old file for a v3 release, which I believe is the next in line, so I say, remove it.

@gugu
Copy link
Contributor Author

gugu commented Feb 7, 2021

@cjbarth done

@gugu gugu merged commit 54809d1 into master Feb 7, 2021
@cjbarth cjbarth deleted the multisaml-strategy-readme branch February 20, 2021 16:32
@cjbarth cjbarth mentioned this pull request May 10, 2021
@cjbarth cjbarth added the documentation Request for or contribution to documentation label May 13, 2021
@markstos
Copy link
Contributor

We just are getting to upgrading to 3.x ourselves and although I read through the CHANGELOG for breaking changes, I missed this. It could use a more clear note in the Changelog that it's a breaking change and a code change is required:

-var MultiSamlStrategy = require('passport-saml/multiSamlStrategy');
+const { MultiSamlStrategy } = require('passport-saml');

However, given the automated Changelog generation I'm not sure how to add that. At least if someone follows a link from the Changelog to here, they'll find the required syntax change more readily.

@cjbarth
Copy link
Collaborator

cjbarth commented Oct 1, 2021

The PR that caused the break can be tagged with breaking-change and then it will get re-organized in the automatic changelog generation.

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

Successfully merging this pull request may close these issues.

None yet

3 participants