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

Fix circular references #879 #880

Merged
merged 6 commits into from
Mar 26, 2024

Conversation

LaurensRietveld
Copy link
Contributor

Description

This fixes #879 . Fixed this by breaking the circular chain, ensuring that we don't import the ./index.js file in other files.
I did not include tests, as this depends on the node version, and whether the package runs in ESM mode

Checklist:

src/strategy.ts Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Dec 8, 2023

This PR is stale because is has been open for 90 days with no activity.

@github-actions github-actions bot added the stale label Dec 8, 2023
@LaurensRietveld
Copy link
Contributor Author

not stale, this is still an issue, and this PR still fixes it

@cjbarth
Copy link
Collaborator

cjbarth commented Dec 8, 2023

The bot is just doing its job. We haven't forgotten about this. We're trying to work through getting node-saml released with the updated version of xml-crypto before we tackle this. Thank you for pinging this to keep it alive.

@github-actions github-actions bot removed the stale label Dec 9, 2023
@kamronbatman
Copy link

kamronbatman commented Feb 6, 2024

xml-crypto v6 came out, does that have the fix? (I am not sure what fix you are waiting for, so I am sorry for the ignorance).

@cjbarth
Copy link
Collaborator

cjbarth commented Feb 6, 2024

Yea, that contains part of the fix. I'm working on getting it into node-saml. Once that comes out, then we're on to this one. Should be soon; just waiting on some PRs to be signed off on.

Copy link

codecov bot commented Mar 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.42%. Comparing base (430d94e) to head (595c6cc).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #880   +/-   ##
=======================================
  Coverage   64.42%   64.42%           
=======================================
  Files           4        4           
  Lines         149      149           
  Branches       37       37           
=======================================
  Hits           96       96           
  Misses         30       30           
  Partials       23       23           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cjbarth
Copy link
Collaborator

cjbarth commented Mar 6, 2024

@LaurensRietveld , this PR is the next in line to get landed. Can you think of anyway to test this to make sure that the problem doesn't re-appear?

@LaurensRietveld
Copy link
Contributor Author

@LaurensRietveld , this PR is the next in line to get landed. Can you think of anyway to test this to make sure that the problem doesn't re-appear?

Good to hear!

I thought about testing this properly, but think that's intricate: my problem only occurs when 1) there is a circular dependency, and 2) when importing node-saml from an ESM module.
Automating this is tricky.

@cjbarth cjbarth merged commit a06ff72 into node-saml:master Mar 26, 2024
8 checks passed
@cjbarth cjbarth added the bug label Mar 26, 2024
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.

Importing passport-saml fails on node v20.6.0
3 participants