[SDK-1640] Remove the Types namespace
#1518
Conversation
github-actions
bot
temporarily deployed
to
staging/pull/1518/features
github-actions
bot
temporarily deployed
to
staging/pull/1518/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1518/typedoc
There was a problem hiding this comment.
Looks good on the whole, I do agree though that the naming should be consistent so RealtimePresenceModule isn't great. tbh I can't think of a better way to name them to avoid a conflict but I do wonder - do we really need to export all the types from both 'ably/modules' and 'ably' separately? If someone only imports types from 'ably' then they will all get stripped by the compiler anyway so it's not like we need a separate tree-shakable way to access the types?
I guess I thought that there were two separate variants of the library, and you used one or the other. You mean you think it'd be better for the user to just import the modules-specific stuff from I'm a bit confused by the word "but" though — is there a connection between your question and the naming of |
|
Thinking about it more, the types for the modular version are closely based on the way we did them for the previous Promise-based variant of the library. Correspondingly, we also have two separate versions of the TypeDoc documentation, as if they were two separate libraries. If we were to only export the types from the default variant of the library, I'm not sure what that would mean for the TypeDoc documentation for the modular variant of the library — like, where would the links to the referenced types point to if these types are not exported by the library and hence not contained in the documentation? |
Yeah I mean it's the same library at the end of the day, just different entry points for importing code. I think it's fine to import types from
That I meant was that if the
OK I don't know the workings of our typedoc setup but I'm not a fan of having this one thing named differently from everything else so if we can find a solution to this I would prefer that, do you think it's def not possible? |
lawrence-forooghian
force-pushed
the
909-remove-Types-namespace
branch
from
4b3a1c6 to
d42ccef
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1518/features
github-actions
bot
temporarily deployed
to
staging/pull/1518/bundle-report
lawrence-forooghian
force-pushed
the
909-remove-Types-namespace
branch
from
d42ccef to
ac4f118
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1518/features
github-actions
bot
temporarily deployed
to
staging/pull/1518/bundle-report
Missed this in b839715.
To avoid a clash with the top-level class of the same name when we remove the Types namespace in #909.
To avoid a clash with the top-level classes of the same name when we remove the Types namespace in #909.
When we remove the Types namespace in #909, the RealtimePresence class defined in ably.d.ts will clash with the variable of the same name declared in modules.d.ts. Owen indicated that his preference for resolving this issue is to export the common types (i.e. those currently contained in the Types namespace) from only the default variant and not from the modular variant, so that’s what we do here. Users of the modular variant who wish to refer to the common types will need to import them from the default variant. To maintain TypeDoc’s ability to generate hyperlinks from the references which are contained in the modular variant but which refer to the common types (e.g. some @link tags, BaseRest’s inheritance from AbstractRest), we switch to using a single TypeDoc project to generate documentation for both variants. For reasons that I don’t understand, TypeDoc is unable to resolve the aforementioned @link tags unless I explicitly add a module source [1]. I’d have thought that it’d be sufficient to simply `import` the types that these tags refer to, but doing so makes no difference. So, I’ve added these module sources. Unfortunately, IntelliSense in VS Code doesn’t know what to do with these and the link is not clickable. (I also tried experimenting with TSDoc’s syntax for declaration references [2] but it made no difference). I think we’ll have to live with this; luckily there are only a few such tags. [1] https://typedoc.org/guides/declaration-references/#module-source [2] https://tsdoc.org/pages/tags/link/
We’re removing it because we believe that it’s more consistent with other JS libraries for users to write `import * as Ably from 'ably'` and then access types on the `Ably` namespace. The triggering of @typescript-eslint/no-redeclare (and hence the added eslint-disable-next-line statements) seems to be related to [1]. I’m not sure why it wasn’t complaining before, though. Resolves #909. [1] typescript-eslint/typescript-eslint#2818
lawrence-forooghian
force-pushed
the
909-remove-Types-namespace
branch
from
ac4f118 to
30f267d
Compare
|
OK, I've dropped the |
|
Tests are all failing, I believe it's sandbox related. Will re-run once told it's back up. |
We’re removing it because we believe that it’s more consistent with other JS libraries for users to write
import * as Ably from 'ably'and then access types on theAblynamespace.See commit messages for more details. In particular, I'd welcome feedback on the rename of the tree-shakable
RealtimePresencemodule toRealtimePresenceModuleto avoid a clash, which I’m not very happy about.Resolves #909.