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

Maybe dropping thecodingmachine/safe as a dependency #2649

Closed
stayallive opened this issue Jan 7, 2025 · 8 comments · Fixed by #2657
Closed

Maybe dropping thecodingmachine/safe as a dependency #2649

stayallive opened this issue Jan 7, 2025 · 8 comments · Fixed by #2657
Labels
enhancement A feature or improvement

Comments

@stayallive
Copy link
Collaborator

Lighthouse (and laragraph/utils, mll-lab/graphql-php-scalars and possibly others) are requiring thecodingmachine/safe as a dependency. However this project does not seem to be maintained (anymore) and its benefit also not that great anymore. The biggest issue is that it't not 8.4 compatible causing a lot of deprecation notices, although silence able I wonder if we could just remove the dependency altogether?

From what I can tell at least in this project most of it's usage is around testing and most other code might need a few extra lines to throw on an error themself but seems do-able.

Search for usage: https://github.com/search?q=repo%3Anuwave%2Flighthouse+%22Safe%5C%5C%22&type=code&p=1

WDYT?

@spawnia
Copy link
Collaborator

spawnia commented Jan 7, 2025

The package actually seems to be picking up steam again lately, see thecodingmachine/safe#499.

I do see the value in minimizing dependencies regardless. The usages in this library are somewhat limited, so I definitely see it as something realistic. However, wouldn't we need to get rid of the transitive dependency in all other packages too to realize the benefit?

@spawnia spawnia added the enhancement A feature or improvement label Jan 7, 2025
@stayallive
Copy link
Collaborator Author

You are right of course but it seems like this might be do-able. It's not used in a third party dependency as far as I can tell (as in you are maintainer in all of them except the phpstan rule which can be dropped):

~/Developer/lighthouse master ❯ composer why thecodingmachine/safe                                                                                                      
nuwave/lighthouse                  dev-master requires thecodingmachine/safe (^1 || ^2)
laragraph/utils                    v2.1.0     requires thecodingmachine/safe (^1.1 || ^2)
mll-lab/graphql-php-scalars        v6.3.0     requires thecodingmachine/safe (^1.3 || ^2)
thecodingmachine/phpstan-safe-rule v1.2.0     requires thecodingmachine/safe (^1.0 || ^2.0)

We can also postpone this to see what the rekindled effort might do of course.

@YoranBosman
Copy link

YoranBosman commented Feb 12, 2025

thecodingmachine 3.0.0 has fixed the php 8.4 compatibility, but this needs a fix in composer.json
"thecodingmachine/safe": "^1 || ^2", to allow 3

@stayallive
Copy link
Collaborator Author

Awesome! This looks like a easy constraint to bump, good work!

@spawnia
Copy link
Collaborator

spawnia commented Feb 12, 2025

I am working on updating the constraint in the dependencies.

@spawnia
Copy link
Collaborator

spawnia commented Feb 14, 2025

I have begun work on #2657.

@spawnia
Copy link
Collaborator

spawnia commented Feb 17, 2025

@stayallive
Copy link
Collaborator Author

Thank you very much for the hard work! My console thanks you very much! 🧡

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

Successfully merging a pull request may close this issue.

3 participants