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.

Sign up for GitHub

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

Jump to bottom

v2: add deprecation warnings for removed and not-previously-deprecated client options #1666

Closed
SimonWoolf opened this issue Mar 5, 2024 · 7 comments
Closed

v2: add deprecation warnings for removed and not-previously-deprecated client options #1666

SimonWoolf opened this issue Mar 5, 2024 · 7 comments
Assignees
@lawrence-forooghian

Comments

@SimonWoolf
Copy link
Member

SimonWoolf commented Mar 5, 2024 •
edited by sync-by-unito bot
Loading

like clientOptions.log

┆Issue is synchronized with this Jira Task by Unito
@lawrence-forooghian
Copy link
Collaborator

What do you mean by "add deprecation warnings" here? Do you mean to add warnings into v1 of ably-js?

@SimonWoolf
Copy link
Member Author

I was mainly thinking, if someone's doing eg new Ably.Realtime({key: xxx, log: {level: 3, handler: <some handler>}}) currently -- and say they're using js not ts, so no type warnings -- then when they upgrade to v2, that will silently stop working with no messages or anything. (ultimately because we don't validate client options)

@lawrence-forooghian
Copy link
Collaborator

lawrence-forooghian commented Mar 7, 2024 •
edited
Loading

OK, that is indeed not ideal. Is your suggestion that v2 emit a log message warning the user that they've provided a client option that's no longer used by the library, and explaining what they should use instead?

@SimonWoolf
Copy link
Member Author

yeah exactly

@lawrence-forooghian lawrence-forooghian self-assigned this Mar 7, 2024
@lawrence-forooghian
Copy link
Collaborator

I wonder whether it might be more consistent with the existing approach if we instead release a new version of 1.x that adds deprecation warnings for this stuff (and introduces the replacement API where possible, e.g. adding logHandler and logLevel alongside log). Then in our migration guide for v2, our first piece of advice can just be "start by upgrading to the latest version of 1.x, then run and address all the deprecation warnings". What do you think?

@lawrence-forooghian
Copy link
Collaborator

(To be clear, this would be in addition to the migration guide also explaining how to migrate away from all of the API removed in v2.)

lawrence-forooghian added a commit that referenced this issue Mar 7, 2024
@lawrence-forooghian
Add spec-conforming logging API
5d52640 
Cherry-picked from commit eaee6f6 in the integration/v2 branch, but
preserving the old API with a deprecation warning. Message of that
commit:

> Conform to spec for logging configuration
>
> Replace the ClientOptions.log property with separate logLevel and
> logHandler properties, to conform with TO3b and TO3c.
>
> Resolves #642.

Decided to add the new API to v1 so that we can add a deprecation
warning to the old one before removing it in v2. This is the approach
that we’ve taken with all the other client options that v2 removes.

Resolves #1666.
@lawrence-forooghian lawrence-forooghian mentioned this issue Mar 7, 2024
Merged
lawrence-forooghian added a commit that referenced this issue Mar 7, 2024
@lawrence-forooghian
Add spec-conforming logging API
f2cfecf 
Cherry-picked from commit eaee6f6 in the integration/v2 branch, but
preserving the old API with a deprecation warning. Message of that
commit:

> Conform to spec for logging configuration
>
> Replace the ClientOptions.log property with separate logLevel and
> logHandler properties, to conform with TO3b and TO3c.
>
> Resolves #642.

Decided to add the new API to v1 so that we can add a deprecation
warning to the old one before removing it in v2.

Part of #1666.
@lawrence-forooghian
Copy link
Collaborator

#1671 implements what I suggested above for the logging client options. Will look what other options need deprecation warnings.

@lawrence-forooghian lawrence-forooghian mentioned this issue Mar 7, 2024
Closed
lawrence-forooghian added a commit that referenced this issue Mar 7, 2024
@lawrence-forooghian
Add spec-conforming logging API
065a402 
Cherry-picked from commit eaee6f6 in the integration/v2 branch, but
preserving the old API with a deprecation warning. Message of that
commit:

> Conform to spec for logging configuration
>
> Replace the ClientOptions.log property with separate logLevel and
> logHandler properties, to conform with TO3b and TO3c.
>
> Resolves #642.

Decided to add the new API to v1 so that we can add a deprecation
warning to the old one before removing it in v2. This will then allow
the first step our our v2 migration guide to simply be “upgrade to the
latest version of v1 and address all the deprecation warnings”.

Part of #1666.
lawrence-forooghian added a commit that referenced this issue Mar 7, 2024
@lawrence-forooghian
Add spec-conforming logging API
36162c4 
Cherry-picked from commit eaee6f6 in the integration/v2 branch, but
preserving the old API with a deprecation warning. Message of that
commit:

> Conform to spec for logging configuration
>
> Replace the ClientOptions.log property with separate logLevel and
> logHandler properties, to conform with TO3b and TO3c.
>
> Resolves #642.

Decided to add the new API to v1 so that we can add a deprecation
warning to the old one before removing it in v2. This will then allow
the first step of our v2 migration guide to simply be “upgrade to the
latest version of v1 and address all the deprecation warnings”.

Part of #1666.
@lawrence-forooghian lawrence-forooghian mentioned this issue Mar 8, 2024
Closed
lawrence-forooghian added a commit that referenced this issue Mar 8, 2024
@lawrence-forooghian
Add a deprecation warning for the headers client option
4985644 
Along with an explanation of why it’s in DeprecatedClientOptions.

We’ve removed this client option in commit d7aaf34 on the integration/v2
branch. It sounds unlikely this will affect anyone, but add a
deprecation warning just in case, and to be consistent with our approach
to other deprecated client options.

TODO improve logging for deprecated client options

Part of #1666.
lawrence-forooghian added a commit that referenced this issue Mar 8, 2024
@lawrence-forooghian
Add a deprecation warning for the headers client option
c3edfb2 
Along with an explanation of why it’s in DeprecatedClientOptions.

We’ve removed this client option in commit d7aaf34 on the integration/v2
branch. It sounds unlikely this will affect anyone, but add a
deprecation warning just in case, and to be consistent with our approach
to other deprecated client options.

Part of #1666.
@lawrence-forooghian lawrence-forooghian mentioned this issue Mar 8, 2024
Merged
@sync-by-unito sync-by-unito bot closed this as completed Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lawrence-forooghian lawrence-forooghian

Labels
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@SimonWoolf @lawrence-forooghian