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

Sort out the false class exports in our type declarations #1519

Closed
lawrence-forooghian opened this issue Nov 30, 2023 · 2 comments
Closed

Sort out the false class exports in our type declarations #1519

lawrence-forooghian opened this issue Nov 30, 2023 · 2 comments
Assignees
@lawrence-forooghian
Labels
breaking Backwards incompatible changes made to the public API. bug Something isn't working. It's clear that this does need to be fixed.

Comments

@lawrence-forooghian
Copy link
Collaborator

lawrence-forooghian commented Nov 30, 2023 •
edited
Loading

(This issue is written with respect to the state of the type declarations after the changes made in #1518, but it applied equally before those changes, too.)

The type declarations claim that the SDK exports all manner of classes that it does not in fact export, such as Auth, Presence, PushAdmin, AbstractRest, AbstractRealtime etc. Trying to import these classes will result in a runtime (or bundler) error.

We should fix this.

My suggestion would be to change these classes to interfaces. I think that only place where this will present a bit of a problem is with AbstractRest and AbstractRealtime; if we change those to interfaces then we'll have to copy all of their method declarations to the Rest and Realtime class declarations in ably.d.ts, and the BaseRest and BaseRealtime class declarations in modules.d.ts. That in itself is not much of an issue, but I’m not sure whether we'd also have to copy all of the documentation comments as well, or whether they'd somehow be inherited (would need to check the behaviour in TypeDoc and also in IntelliSense e.g. in VS Code). We'd also need to consider what to name them; the Abstract prefix probably wouldn't make sense any more, but we can't just call them Rest and Realtime because that clashes with the exports of ably.d.ts. Would we need to stick the word I or Interface in there somewhere?
@sync-by-unito Sync by Unito
Copy link

sync-by-unito bot commented Nov 30, 2023

➤ Automation for Jira commented:

The link to the corresponding Jira issue is https://ably.atlassian.net/browse/SDK-3967

@lawrence-forooghian lawrence-forooghian mentioned this issue Nov 30, 2023
Closed
@lawrence-forooghian lawrence-forooghian added bug Something isn't working. It's clear that this does need to be fixed. breaking Backwards incompatible changes made to the public API. labels Nov 30, 2023
@lawrence-forooghian lawrence-forooghian self-assigned this Dec 1, 2023
lawrence-forooghian added a commit that referenced this issue Dec 4, 2023
@lawrence-forooghian
Remove false class exports in type declarations
572cfa8 
The type declarations claim that the SDK exports all manner of classes
that it does not in fact export, such as Auth, Presence, PushAdmin,
AbstractRest, AbstractRealtime etc. Trying to import these classes will
result in a runtime (or bundler) error.

So, we convert them all to interfaces. (I can’t see an obvious downside
of doing this; let me know if there is.)

Resolves #1519.
@lawrence-forooghian lawrence-forooghian mentioned this issue Dec 4, 2023
Merged
lawrence-forooghian added a commit that referenced this issue Dec 4, 2023
@lawrence-forooghian
Remove false class exports in type declarations
7392200 
The type declarations claim that the SDK exports all manner of classes
that it does not in fact export, such as Auth, Presence, PushAdmin,
AbstractRest, AbstractRealtime etc. Trying to import these classes will
result in a runtime (or bundler) error.

So, we convert them all to interfaces. (I can’t see an obvious downside
of doing this; let me know if there is.)

I’ve also addressed the ClientOptions docstring’s links to the Abstract*
constructors by rewording their text, since these links are now
completely broken (they were previously linking to a useless entry for a
nonexistent constructor).

Resolves #1519, resolves #1520.
lawrence-forooghian added a commit that referenced this issue Jan 4, 2024
@lawrence-forooghian
Remove false class exports in type declarations
3f5baed 
The type declarations claim that the SDK exports all manner of classes
that it does not in fact export, such as Auth, Presence, PushAdmin,
AbstractRest, AbstractRealtime etc. Trying to import these classes will
result in a runtime (or bundler) error.

So, we convert them all to interfaces. (I can’t see an obvious downside
of doing this; let me know if there is.)

I’ve also addressed the ClientOptions docstring’s links to the Abstract*
constructors by rewording their text, since these links are now
completely broken (they were previously linking to a useless entry for a
nonexistent constructor).

Resolves #1519, resolves #1520.
lawrence-forooghian added a commit that referenced this issue Jan 15, 2024
@lawrence-forooghian
Remove false class exports in type declarations
ff7cb41 
The type declarations claim that the SDK exports all manner of classes
that it does not in fact export, such as Auth, Presence, PushAdmin,
AbstractRest, AbstractRealtime etc. Trying to import these classes will
result in a runtime (or bundler) error.

So, we convert them all to interfaces. (I can’t see an obvious downside
of doing this; let me know if there is.)

I’ve also addressed the ClientOptions docstring’s links to the Abstract*
constructors by rewording their text, since these links are now
completely broken (they were previously linking to a useless entry for a
nonexistent constructor).

Resolves #1519, resolves #1520.
@lawrence-forooghian
Copy link
Collaborator Author

Resolved by #1524.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lawrence-forooghian lawrence-forooghian

Labels
breaking Backwards incompatible changes made to the public API. bug Something isn't working. It's clear that this does need to be fixed.
Milestone
No milestone
Development

No branches or pull requests
1 participant
@lawrence-forooghian