-
Notifications
You must be signed in to change notification settings - Fork 140
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
Conversation
This comment was marked as duplicate.
This comment was marked as duplicate.
There was a problem hiding this 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.
What is the use case for the token parameter because I don’t see a need for it? |
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. |
There was a problem hiding this 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.
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. |
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. |
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. |
Could we work out what that parameter is actually for? |
There was a problem hiding this 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?
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.