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: make sure all the dependencies are checked when flattening the d… #251

Merged

Conversation

danionescu
Copy link
Contributor

…ependencies tree (#103)

This needs more testing. For example, my own project, I am getting the error Error: Cannot find module 'source-map-support/register', which I have seen before my fix.

@samchungy
Copy link
Collaborator

Would a package version affect the dependencies we need?

Eg. If one package had

[email protected] as a dep

And then another had

[email protected] as a dep

@danionescu
Copy link
Contributor Author

I am not sure how that would work with the current code. I pushed another commit that saves the dependencies for each version.

@samchungy
Copy link
Collaborator

I am not sure how that would work with the current code. I pushed another commit that saves the dependencies for each version.

Neither tbh, was just curious if you knew aha 😅

@danionescu
Copy link
Contributor Author

danionescu commented Jan 10, 2022

Lol, I took a bottom up approach to fixing this, assuming the rest of the code is correct, so I just tried to add the pieces that were missing in this place. I would need to study the rest of the code to make sure it is correct, but I don't think that I will have the time soon.

Copy link
Collaborator

@samchungy samchungy left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks for the contribution @danionescu! Sorry it took so long to review. Been very busy 😮‍💨

I answered my above question with this helpful medium article. https://medium.com/learnwithrahul/understanding-npm-dependency-resolution-84a24180901b

I did give this a run locally and it lgtm. It brings across the correct dependencies.

@samchungy
Copy link
Collaborator

@olup @floydspace want to give this a final look?

@floydspace floydspace merged commit 573385c into floydspace:master Feb 1, 2022
@floydspace
Copy link
Owner

I pushed the button, thank you @danionescu @samchungy

@github-actions
Copy link

github-actions bot commented Feb 1, 2022

🎉 This PR is included in version 1.23.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

3 participants