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

[SDK-3768] Add tree-shakable Base* and Rest exports #1417

Merged
merged 13 commits into from
Nov 6, 2023

Conversation

lawrence-forooghian
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian commented Aug 2, 2023

This PR replaces #1400. It copies a lot from that PR but uses the class hierarchy described by #1415.

See commit messages for more details.

The output of npm run modulereport is as follows:

BaseRest: 74.28 KiB
BaseRest + Rest: 91.14 KiB
BaseRealtime: 148.03 KiB
BaseRealtime + Rest: 156.68 KiB

(Note that BaseRest is currently somewhat larger than BaseClient in #1415. This is because it includes the transports and EventEmitter, which are included due to this line. This line is absent in #1415 because that PR takes the approach of stripping everything out to add it back in again later, whereas I've taken the approach of keeping everything as it is until we explicitly decide to split it out. We'll be splitting out the transports in #1394.)

Resolves #1415.

@github-actions github-actions bot temporarily deployed to staging/pull/1417/features August 2, 2023 21:15 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1417/bundle-report August 2, 2023 21:16 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1417/typedoc August 2, 2023 21:16 Inactive
@lawrence-forooghian lawrence-forooghian changed the title 1415 tree shaking attempt 2 [SDK-3768] 1415 tree shaking attempt 2 Aug 2, 2023
@lawrence-forooghian lawrence-forooghian changed the title [SDK-3768] 1415 tree shaking attempt 2 [SDK-3768] Add tree-shakable Base* and Rest exports Aug 2, 2023
@lawrence-forooghian lawrence-forooghian marked this pull request as ready for review August 3, 2023 00:27
@lawrence-forooghian lawrence-forooghian requested review from owenpearson and removed request for owenpearson August 3, 2023 00:27
@lawrence-forooghian lawrence-forooghian marked this pull request as draft August 3, 2023 08:45
@github-actions github-actions bot temporarily deployed to staging/pull/1417/features August 3, 2023 08:54 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1417/typedoc August 3, 2023 08:55 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1417/bundle-report August 3, 2023 08:55 Inactive
@lawrence-forooghian lawrence-forooghian force-pushed the 1415-tree-shaking-attempt-2 branch from 8382826 to 77e48be Compare August 3, 2023 08:59
@github-actions github-actions bot temporarily deployed to staging/pull/1417/features August 3, 2023 09:00 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1417/bundle-report August 3, 2023 09:01 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1417/typedoc August 3, 2023 09:01 Inactive
@lawrence-forooghian lawrence-forooghian force-pushed the 1415-tree-shaking-attempt-2 branch from 77e48be to 05ca42d Compare August 3, 2023 09:23
@github-actions github-actions bot temporarily deployed to staging/pull/1417/features August 3, 2023 09:23 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1417/typedoc August 3, 2023 09:24 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1417/bundle-report August 3, 2023 09:25 Inactive
@lawrence-forooghian lawrence-forooghian marked this pull request as ready for review August 3, 2023 11:45
Copy link
Member

@owenpearson owenpearson left a comment

Choose a reason for hiding this comment

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

Code changes look fine, I thought we opted not to export a rest client without rest apis though - is there any reason someone would want to use a rest client but not call any rest apis?

@lawrence-forooghian
Copy link
Collaborator Author

Code changes look fine, I thought we opted not to export a rest client without rest apis though - is there any reason someone would want to use a rest client but not call any rest apis?

Yep, you're right. I'll look into what the best way to handle this is — we want a way for BaseRest to include Rest without it being included in its subclass BaseRealtime.

@lawrence-forooghian lawrence-forooghian force-pushed the 1415-tree-shaking-attempt-2 branch from 05ca42d to 214c2cf Compare August 15, 2023 12:45
@github-actions github-actions bot temporarily deployed to staging/pull/1417/features August 15, 2023 12:46 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1417/typedoc August 15, 2023 12:47 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1417/bundle-report August 15, 2023 12:47 Inactive
@lawrence-forooghian lawrence-forooghian force-pushed the 1415-tree-shaking-attempt-2 branch from 214c2cf to afb0235 Compare August 15, 2023 13:19
@lawrence-forooghian lawrence-forooghian force-pushed the 2023-11-01-merge-main-into-v2 branch 8 times, most recently from d26354d to 68bf030 Compare November 3, 2023 15:30
Base automatically changed from 2023-11-01-merge-main-into-v2 to integration/v2 November 6, 2023 19:07
lawrence-forooghian and others added 13 commits November 6, 2023 16:08
We rename the Rest class to BaseClient, introduce a new class BaseRest,
and add the prefix "Base" to the name of the Realtime class.

The BaseRest and BaseRealtime classes will be exported by the upcoming
tree-shakable variant of the library. The Default* classes become the
exports of the current (i.e. non tree-shakable) variant.

As we make the library tree-shakable, we will extract functionality from
the BaseClient and BaseRealtime classes and move them into modules
exported by the tree-shakable variant of the library. Whatever
functionality we remove from the Base* classes will be reintroduced in
their Default* counterparts.

The purpose of the BaseRest class is probably not immediately obvious,
since it adds no functionality on top of BaseClient. However, upon
introducing the tree-shakable version of the library, which will split
the REST functionality into a separate module that Realtime can
optionally use, we will use the BaseRest class to ensure that even in
the tree-shakable version of the library, the BaseRest class always
includes REST functionality.

Resolves #1415.

Co-authored-by: Owen Pearson <[email protected]>
This forms the beginning of the tree-shakable variant of the library
mentioned in 6d25bd2. It currently exports the BaseRest and BaseRealtime
classes.

It’s available to import from 'ably/modules'.

Co-authored-by: Owen Pearson <[email protected]>
The Base* classes now take a map of encapsulated modules which can be
provided through the constructor. These modules work like plugins,
adding features to the client.

Co-authored-by: Owen Pearson <[email protected]>
Emit an error if adding a tree-shakable module does not increase the
bundle size (this means that the module is not being tree-shaken
correctly).
This will avoid confusion when we introduce the tree-shakable module
named `Rest`.

Co-authored-by: Owen Pearson <[email protected]>
tl;dr: there's a circular import here between utils.ts and defaults.ts. in the
following commits this results in a build error, so i'm making utils.ts no
longer depend on defaults.ts.
This moves the following functionality from BaseClient into a new module
named Rest:

- methods that wrap REST endpoints (e.g. `stats`)
- the `request` method
- all functionality accessed via `BaseRest.channels` and `BaseRest.push`

This allows us to now construct a BaseRealtime instance that doesn’t
have REST functionality.

Note that the BaseRest class always includes the Rest module. This comes
after discussions in which we decided that it would be quite useless
without it.

Resolves #1374.

Co-authored-by: Owen Pearson <[email protected]>
These already exist in its superclass BaseClient.
These properties are only used by the tests and are not part of the
public API. Let’s move them out of BaseRealtime to make it more
tree-shakable.
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.

2 participants