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

Crude implementation to allow Promise V3 #27

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

Exanlv
Copy link
Contributor

@Exanlv Exanlv commented May 28, 2024

This is more of a proof of concept than a full PR.

#26 introduces Promise V3 compatibility with the use of a helper. With an implementation like this, no helper is needed.

DiscordPHP will still be able to use Promise V2 by adding a hard Promise V2 dependency to composer.json, and if we include an IDE helper to overwrite the return types in DiscordPHP, nothing will have changed for users of DiscordPHP.

I'd like to hear what you guys think of a solution like this. I want to start using Promise V3, whether that be with #26, this PR, or a v3-compatible fork published on packagist alongside main discordphp-http.

@SQKo
Copy link
Member

SQKo commented Jul 21, 2024

This is interesting approach, but i think since this depends on Composer, would be good to give user choice as a different version/different package for use in production, Do you know any example of non dev-tools library that is using any of the Composer utilities package?

@2colours
Copy link

This is in fact all it would take for the whole DiscordPHP library to support v3 Promises...

@Exanlv
Copy link
Contributor Author

Exanlv commented Nov 19, 2024

This is in fact all it would take for the whole DiscordPHP library to support v3 Promises...

Ratchet/pawl has a dependency on Promise V2, not specified in its composer files however

@2colours
Copy link

Ratchet/pawl has a dependency on Promise V2, not specified in its composer files however

Two things.

  1. Depending on an apparently abandoned package that fails to even specify its own dependencies is issue-worthy in itself. I don't think it's right to just try to "outsleep" this problem. There are alternatives to this package. If there is a chance that a replacement can be adapted, I will even look into it, it's just this is not the kind of investment one would want to do for nothing - like this PR so far...
  2. And here I wanted to point out that it might still work with V3 but no, the use of RejectedPromise is definitely in the way. Anyway, it's much less that this dependency couldn't be fixed, it's rather the whole package that seems to be in a poor and abandoned state.

valzargaming added a commit that referenced this pull request Nov 19, 2024
valzargaming added a commit that referenced this pull request Nov 19, 2024
@valzargaming
Copy link
Member

I attempted to replicate these changes and check for any areas of improvement on my own branch, but it seems to have been entirely unnecessarily. I am fine with merging this as-is as the cleanest possible solution.

@valzargaming valzargaming self-requested a review November 19, 2024 19:49
@valzargaming
Copy link
Member

valzargaming commented Nov 19, 2024

Ratchet/pawl has a dependency on Promise V2, not specified in its composer files however

Two things.

  1. Depending on an apparently abandoned package that fails to even specify its own dependencies is issue-worthy in itself. I don't think it's right to just try to "outsleep" this problem. There are alternatives to this package. If there is a chance that a replacement can be adapted, I will even look into it, it's just this is not the kind of investment one would want to do for nothing - like this PR so far...
  2. And here I wanted to point out that it might still work with V3 but no, the use of RejectedPromise is definitely in the way. Anyway, it's much less that this dependency couldn't be fixed, it's rather the whole package that seems to be in a poor and abandoned state.

@2colours, If you have any changes you'd like to propose, feel free to create a PR to address your concerns. Our team is small, and most of the other maintainers are currently inactive. I work full-time and contribute to several other projects, so progress on this one happens as my availability allows.

@valzargaming
Copy link
Member

I've poked about for a bit and I see no reason to further delay this PR. We can look into other options in the future if needed, but this should preserve functionality between both v2 and v3.

@valzargaming valzargaming merged commit 017322d into discord-php:master Nov 19, 2024
valzargaming added a commit that referenced this pull request Nov 20, 2024
Crude implementation to allow Promise V3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

4 participants