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

Removed token parameter which prevented the function from respecting custom parameter values #792

Merged
merged 5 commits into from
May 18, 2024

Conversation

AEW745
Copy link
Contributor

@AEW745 AEW745 commented Apr 15, 2024

Before when you would retrieve messages, it would use the default parameter values no matter what you put when you use the function. Ex. if you change the messageTab, pageSize, or pageNumber it would only use the default values. Therefore, I removed the undefined parameter to fix it. I also added the jar: jar back because it seems to work fine with it.

I also Fixed the onMessage event where it wouldn't get messages due to change in notification name and message type. I had to add the pageSize parameter and set it to 1 so it will get the latest message because before it was sending 25 messages at a time when the event is called which is also why I had to remove the token parameter in the getMessages file because it wouldn't let me set the pageSize.

I Fixed the onNotification issue that was using a deprecated signalr package and has switched over to @microsoft/signalr.
I updated the package.json file to include the change in the dependency.

nottisa

This comment was marked as duplicate.

nottisa

This comment was marked as duplicate.

@nottisa

This comment was marked as duplicate.

Copy link
Contributor

@nottisa nottisa left a comment

Choose a reason for hiding this comment

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

Here are some possible fixes. Sorry for the previous comments. These commits I'm recommending fixes for were not mentioned in your PR comments though. I also noticed this commit may be a breaking change for those who have filled in a value for the token parameter.

lib/privatemessages/onMessage.js Outdated Show resolved Hide resolved
lib/client/onNotification.js Show resolved Hide resolved
@AEW745
Copy link
Contributor Author

AEW745 commented Apr 15, 2024

Here are some possible fixes. Sorry for the previous comments. These commits I'm recommending fixes for were not mentioned in your PR comments though. I also noticed this commit may be a breaking change for those who have filled in a value for the token parameter.

What is the use case for the token parameter because I don’t see a need for it?

@AEW745 AEW745 requested a review from nottisa April 15, 2024 18:57
@nottisa
Copy link
Contributor

nottisa commented Apr 17, 2024

There's not a use case, but it's still possible that if someone was using this function and put in a throwaway value for the token parameter their code may break when they update the library. Your changes look good, I just need to validate they work which might take a couple days unfortunately.

Copy link
Member

@Neztore Neztore left a comment

Choose a reason for hiding this comment

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

Just having a look and identified a few issues though I see some have been spotted already.
Thank you for the PR! I'm happy to merge once these issues have been fixed, and I have two people confirming these fixes work. The (technically) major change is not a massive concern as I intend to make the next version bump to v5.

However, as a result of this pull request some methods would be broken still (and using the old signal package). Is it possible to move those over too? If this is not something you are interested in I will open an issue - and probably not release a new version until that is done.

lib/client/onNotification.js Show resolved Hide resolved
lib/privatemessages/onMessage.js Show resolved Hide resolved
lib/privatemessages/onMessage.js Show resolved Hide resolved
@Neztore Neztore added bug Something isn't working good first issue Good for newcomers dependencies Update one or more dependencies version labels Apr 17, 2024
@AEW745
Copy link
Contributor Author

AEW745 commented Apr 19, 2024

I will be adding the token parameter back because it seems like it is working with it as well. However, testing without the pageSize parameter causes you to receive the last message instead of the most recent message.

@AEW745
Copy link
Contributor Author

AEW745 commented Apr 19, 2024

Upon further testing adding the token parameter while not using it in the request only works with the first message and doesn't allow anymore messages to be received.

@AEW745
Copy link
Contributor Author

AEW745 commented Apr 19, 2024

I will mark the comments as resolved because attempting to fix those will be a breaking change. However, leaving it how I fixed it seems to work properly.

@AEW745 AEW745 requested a review from Neztore April 19, 2024 10:24
@Neztore
Copy link
Member

Neztore commented Apr 23, 2024

Upon further testing adding the token parameter while not using it in the request only works with the first message and doesn't allow anymore messages to be received.

Could we work out what that parameter is actually for?

Copy link
Member

@Neztore Neztore left a comment

Choose a reason for hiding this comment

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

See above: What does token do?

@Neztore Neztore merged commit d626e6f into noblox:master May 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dependencies Update one or more dependencies version good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants