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

Fix ably request method param #1260

Merged
merged 7 commits into from
Oct 13, 2023
Merged

Fix ably request method param #1260

merged 7 commits into from
Oct 13, 2023

Conversation

sacOO7
Copy link
Collaborator

@sacOO7 sacOO7 commented Sep 15, 2023

  • Fix http request method as per JSON Encoding issues #1255 (comment)
  • Since the external client NewtonSoft version is not guaranteed to be in sync with internal newtonsoft, call to the Request method with external JToken param will always fail.
  • Updated method with string requestbody as a param instead.
  • Since this is an optional param, shouldn't fail for customers upgrading to a new version.

@sacOO7 sacOO7 requested a review from owenpearson September 15, 2023 11:08
@github-actions github-actions bot temporarily deployed to staging/pull/1260/features September 15, 2023 11:08 Inactive
@sacOO7 sacOO7 mentioned this pull request Sep 15, 2023
@github-actions github-actions bot temporarily deployed to staging/pull/1260/features September 15, 2023 11:29 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1260/features September 15, 2023 17:02 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1260/features September 15, 2023 19:30 Inactive
@sacOO7 sacOO7 force-pushed the fix/http-batch-request branch from dfdd8b6 to 5a196b2 Compare September 15, 2023 19:36
@github-actions github-actions bot temporarily deployed to staging/pull/1260/features September 15, 2023 19:37 Inactive
Copy link

@fdmarc fdmarc left a comment

Choose a reason for hiding this comment

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

This is a breaking change - you are changing the signature of Ably.Request, which is public API. I'm not sure what your policy is for such SDK changes?

@sacOO7
Copy link
Collaborator Author

sacOO7 commented Sep 27, 2023

This is a breaking change - you are changing the signature of Ably.Request, which is public API. I'm not sure what your policy is for such SDK changes.

The policy is releasing a major version for breaking changes as per semvar.

Generally, we create a new public method and mark the old one as deprecated. Though I tried the same approach, seems we can't do it without changing the method name : (
I was pushing for keeping the old name for usability since this method fails while passing param ( JObject ), I don't think there are customers that really use it.

@fdmarc
Copy link

fdmarc commented Sep 27, 2023

Merging as-is would help me out a bunch, since I can then move to making batch calls to your REST API 💪

@github-actions github-actions bot temporarily deployed to staging/pull/1260/features September 27, 2023 15:32 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1260/features September 27, 2023 15:38 Inactive
@sacOO7 sacOO7 force-pushed the fix/http-batch-request branch from b64da5c to 17d58ae Compare September 27, 2023 15:38
@github-actions github-actions bot temporarily deployed to staging/pull/1260/features September 27, 2023 15:39 Inactive
@sacOO7 sacOO7 force-pushed the fix/http-batch-request branch from 17d58ae to 9e7b0d9 Compare September 27, 2023 15:40
@github-actions github-actions bot temporarily deployed to staging/pull/1260/features September 27, 2023 15:41 Inactive
@sacOO7 sacOO7 requested a review from ttypic September 27, 2023 15:41
@sacOO7 sacOO7 force-pushed the fix/http-batch-request branch from 9e7b0d9 to 110ae69 Compare September 27, 2023 15:45
@github-actions github-actions bot temporarily deployed to staging/pull/1260/features September 27, 2023 15:46 Inactive
Copy link
Member

@owenpearson owenpearson left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link

@ttypic ttypic left a comment

Choose a reason for hiding this comment

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

LGTM

@fdmarc
Copy link

fdmarc commented Oct 9, 2023

@sacOO7 can we get this released to Nuget please?

@sacOO7
Copy link
Collaborator Author

sacOO7 commented Oct 13, 2023

@sacOO7 can we get this released to Nuget please?

@fdmarc , sure we will do it soon 👍

@sacOO7 sacOO7 merged commit 42a100a into main Oct 13, 2023
5 of 9 checks passed
@sacOO7 sacOO7 deleted the fix/http-batch-request branch October 13, 2023 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants