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-3717] Add tree-shakable BaseClient and Rest exports #1400

Closed
wants to merge 5 commits into from

Conversation

owenpearson
Copy link
Member

@owenpearson owenpearson commented Jul 19, 2023

Resolves #1374

Refactors the existing Rest class into BaseClient and an encapsulated Rest class which can be passed in to the BaseClient constructor to allow access to the rest interface.

The tree-shakable modules are currently available as an esm module from 'ably/modules', example usage:

import { BaseClient, Rest } from 'ably/modules';

const client = new BaseClient(options, { Rest });

A new npm script (npm run modulereport) is added which displays information about the size of each module in the bundle-report workflow. Currently (76d2ab0) the output shows:

BaseClient: 61.63kb
BaseClient + Rest: 78.56kb

These numbers will be lower after we encapsulate further functionality into tree-shakable modules.

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 commit adds:
- A minimal BaseClient which takes a map of encapsulated modules which
  can be provided through the constructor. These modules work like
  plugins, adding features to the BaseClient.
- An encapsulated Rest module/plugin which is compatible with the
  BaseClient

The existing Rest class is moved to DefaultRest and is refactored to use
the BaseClient and new Rest module
@github-actions github-actions bot temporarily deployed to staging/pull/1400/features July 19, 2023 09:59 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1400/bundle-report July 19, 2023 10:00 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1400/typedoc July 19, 2023 10:00 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1400/features July 19, 2023 10:04 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1400/bundle-report July 19, 2023 10:05 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1400/typedoc July 19, 2023 10:05 Inactive
Base automatically changed from esbuild to integration/v2 July 19, 2023 10:06
@owenpearson owenpearson marked this pull request as ready for review July 19, 2023 10:12
@github-actions github-actions bot temporarily deployed to staging/pull/1400/features July 19, 2023 10:13 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1400/typedoc July 19, 2023 10:13 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1400/bundle-report July 19, 2023 10:14 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1400/features July 19, 2023 14:12 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1400/bundle-report July 19, 2023 14:13 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1400/typedoc July 19, 2023 14:13 Inactive
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian left a comment

Choose a reason for hiding this comment

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

I'm approving (with one comment), but as discussed offline, I'm still not sure whether it makes sense to combine the realtime and REST functionality into a single BaseClient class. My preference would be to resolve this matter before merging this PR, but if you'd prefer to defer the decision until a bit later then that's cool too 👍

src/common/lib/client/realtime.ts Show resolved Hide resolved
@jamienewcomb
Copy link
Member

@owenpearson can we remember to link PRs to the jira issue they are fixing please. Happy to have a discussion about why this is required need be.

It can be done by tagging the issue key in the title. see ably/ably-go#596
Or by adding it to the PR description. see ably/ably-java#966

This ask is now reflected in: https://github.com/ably/engineering/blob/main/best-practices/pull-requests.md

@github-actions github-actions bot temporarily deployed to staging/pull/1400/features August 1, 2023 13:36 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1400/typedoc August 1, 2023 13:37 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1400/bundle-report August 1, 2023 13:37 Inactive
@owenpearson owenpearson changed the title Add tree-shakable BaseClient and Rest exports [SDK-3717] Add tree-shakable BaseClient and Rest exports Aug 1, 2023
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we update the format task to include this file?

@lawrence-forooghian
Copy link
Collaborator

Replaced by #1417.

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