-
Notifications
You must be signed in to change notification settings - Fork 124
[Node SDK] consistent handling of timeout values for constructor and overrides #634
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
[Node SDK] consistent handling of timeout values for constructor and overrides #634
Conversation
|
After discussing this with @AaronDDM we both agree implementing this right away would be a breaking change, we're going to handle this by:
|
src/apiClient.ts
Outdated
| // eslint-disable-next-line no-console | ||
| console.warn( | ||
| 'DEPRECATED: Providing timeout in milliseconds via request overrides is deprecated and will be removed in the next major release. Please use seconds instead.' | ||
| ); |
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.
@ajay-k let's remove this console warn message and instead add a @deprecated [message here] to the options.overrides.timeout property defined in the OverridableNylasConfig interface/type. So we'll just use typescript to notify of this impending deprecation.
Also update the CHANGELOG.md to call out this deprecation.
Finally, you have some github actions failing that need to be resolved.
Thank you!
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.
Hey @AaronDDM made some changes you suggested, can you review and lmk if it looks good to you as well?
…s-milliseconds-in-node-sdk-7-8-0
Description
This pull request fixes an issue where request timeout overrides were inconsistently handled compared to the timeout set in the constructor. Previously, the timeout in the constructor was treated as seconds and converted to milliseconds, while timeout in request overrides was incorrectly treated directly as milliseconds.
The changes in this PR ensure that:
Consistent behavior with backward compatibility:
Correct error reporting:
Error messages correctly display the actual timeout value that was used for the request (in seconds)
Previously it would always show the client timeout regardless of which timeout was used
These changes maintain API compatibility while fixing the inconsistent behavior, providing a clear migration path for users. After the next major version, all timeout values will be treated consistently as seconds.
License
I confirm that this contribution is made under the terms of the MIT license and that I have the authority necessary to make this contribution on behalf of its copyright owner.