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

Move GoodSignal from Service.lua to its own file #1244

Merged
merged 4 commits into from
Nov 9, 2023

Conversation

ccuser44
Copy link
Contributor

@ccuser44 ccuser44 commented Nov 8, 2023

Move GoodSignal from Service.lua to its own file and add license to comply with law

@Dimenpsyonal Dimenpsyonal added the 🎏 miscellaneous Miscellaneous content label Nov 8, 2023
@coasterteam
Copy link
Collaborator

Lacks the additional backwards compatibility that was setup with the in-Service version, will review with changes later today when I get time

@Dimenpsyonal
Copy link
Member

Indeed

@ccuser44
Copy link
Contributor Author

ccuser44 commented Nov 8, 2023

Lacks the additional backwards compatibility that was setup with the in-Service version, will review with changes later today when I get time

I did a diffcheck and the only changes were the removals of Signal:ConnectOnce, Signal:connectOnce, Signal:wait, Signal:fire, Signal:connect, connection:Fire, connection:fire, connection:disconnect, connection:wait, Event = {}, and Signal:Destroy.
I am not sure why the removal of these would make the compatibility any better so I decided to keep them.

@coasterteam
Copy link
Collaborator

This was due to GoodSignal being the replacement of the old wrapped BindableEvents. They had those methods existing so I didn't want to break existing events someone might have/use.

@ccuser44
Copy link
Contributor Author

ccuser44 commented Nov 8, 2023

This was due to GoodSignal being the replacement of the old wrapped BindableEvents. They had those methods existing so I didn't want to break existing events someone might have/use.

But wouldn't removal exactly break said existing events? This pull doesn't remove anything but the one in Service.lua does.

@coasterteam
Copy link
Collaborator

I modified GoodSignal with additional methods as wrapped BindableEvents were removed from the Events system. I am not sure how this is confusing?

@ccuser44
Copy link
Contributor Author

ccuser44 commented Nov 8, 2023

I modified GoodSignal with additional methods as wrapped BindableEvents were removed from the Events system. I am not sure how this is confusing?

So do I remove the added methods?

@coasterteam coasterteam self-requested a review November 8, 2023 18:30
@coasterteam
Copy link
Collaborator

Added reviewed changes to ensure backwards compatibility with the events system this was replacing.

@ccuser44
Copy link
Contributor Author

ccuser44 commented Nov 9, 2023

Added reviewed changes to ensure backwards compatibility with the events system this was replacing.

I merged the suggestions now.

I somehow thought that the additions were actually in the vanilla GoodSignal and were removed in Adonis for compatibility but apparently it was the other way around.

Anyways it's fixed now.

@Dimenpsyonal
Copy link
Member

Very cool

@Dimenpsyonal Dimenpsyonal merged commit 482357d into Epix-Incorporated:master Nov 9, 2023
2 checks passed
@ccuser44 ccuser44 deleted the patch-8 branch November 9, 2023 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎏 miscellaneous Miscellaneous content
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants