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

Add typings for ClientOptions #854

Merged

Conversation

owenpearson
Copy link
Member

No description provided.

@github-actions github-actions bot temporarily deployed to staging/pull/854/bundle-report November 5, 2021 15:37 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/854/bundle-report November 8, 2021 13:01 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/854/bundle-report November 8, 2021 14:32 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/854/bundle-report November 8, 2021 15:11 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/854/bundle-report November 8, 2021 15:36 Inactive
Copy link

@ikbalkaya ikbalkaya left a comment

Choose a reason for hiding this comment

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

I realised lots of usage of optional properties in this PR. I think they make things flexible but also make code a bit more vulnerable. (correct me if this is not the case in Javascript) my understanding of optional usage is that they should be used with a purpose and choosing them is part of design of the app. For example, by design the autoconnect property is intended to be true by default, but making it optional you declare it so that it may not even exist. I hope that is helpful.

common/types/AuthOptions.ts Show resolved Hide resolved
common/types/ClientOptions.ts Show resolved Hide resolved
common/types/ClientOptions.ts Show resolved Hide resolved
Copy link
Contributor

@QuintinWillison QuintinWillison left a comment

Choose a reason for hiding this comment

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

I acknowledge that it's perhaps unrealistic to try to add API commentary to every type as part of the JS to TS conversion, however I think new types should be easy to document now.

common/types/utils.d.ts Show resolved Hide resolved
@QuintinWillison QuintinWillison dismissed ikbalkaya’s stale review November 29, 2021 12:41

Comments have been addressed and this reviewer is not available today to approve for themselves.

Base automatically changed from feature/channel-typescript to integration/typescript December 1, 2021 12:31
@owenpearson owenpearson merged commit 566efbd into integration/typescript Dec 1, 2021
@owenpearson owenpearson deleted the feature/clientoptions-typescript branch December 1, 2021 12:31
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.

3 participants