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-3938] Remove the existing plugins mechanism #1512

Merged
merged 8 commits into from
Nov 29, 2023

Conversation

lawrence-forooghian
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian commented Nov 21, 2023

This removes the ClientOptions.plugins mechanism and moves the Vcdiff functionality to a new tree-shakable Vcdiff module. See commit messages for more details.

Resolves #1492.

@github-actions github-actions bot temporarily deployed to staging/pull/1512/features November 21, 2023 20:11 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/bundle-report November 21, 2023 20:12 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/typedoc November 21, 2023 20:12 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/features November 21, 2023 20:13 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/bundle-report November 21, 2023 20:14 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/typedoc November 21, 2023 20:14 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/features November 21, 2023 20:25 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/bundle-report November 21, 2023 20:25 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/typedoc November 21, 2023 20:25 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/features November 22, 2023 13:28 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/typedoc November 22, 2023 13:29 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/bundle-report November 22, 2023 13:29 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/features November 22, 2023 14:01 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/bundle-report November 22, 2023 14:02 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/typedoc November 22, 2023 14:02 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/features November 22, 2023 14:54 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/bundle-report November 22, 2023 14:55 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/typedoc November 22, 2023 14:55 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/features November 22, 2023 14:56 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/bundle-report November 22, 2023 14:57 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/typedoc November 22, 2023 14:57 Inactive
@lawrence-forooghian lawrence-forooghian changed the title [SDK-3938, WIP] Remove the existing plugins mechanism [SDK-3938] Remove the existing plugins mechanism Nov 22, 2023
@lawrence-forooghian lawrence-forooghian force-pushed the 1492-remove-plugins-mechanism branch 2 times, most recently from 6fa63dc to b89b69a Compare November 27, 2023 20:29
@github-actions github-actions bot temporarily deployed to staging/pull/1512/features November 27, 2023 20:40 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/bundle-report November 27, 2023 20:41 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/typedoc November 27, 2023 20:41 Inactive
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.

LGTM, nice 👍

@github-actions github-actions bot temporarily deployed to staging/pull/1512/features November 28, 2023 17:11 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/bundle-report November 28, 2023 17:11 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/typedoc November 28, 2023 17:12 Inactive
I suspect this was a copy-and-paste error in a7d9a5e.
Make it match the signature of @ably/vcdiff-decoder’s `decode`.
Fix inverted code and statusCode, and use the "Required client library
plugin not present" [1] error code (which we currently use when the
vcdiff plugin is missing).

[1] https://github.com/ably/ably-common/blob/main/protocol/errors.json
I’m going to use them inside `modules.test.js` as part of #1492
(removing the ClientOptions.plugins mechanism).
@github-actions github-actions bot temporarily deployed to staging/pull/1512/features November 28, 2023 17:29 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/bundle-report November 28, 2023 17:30 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1512/typedoc November 28, 2023 17:30 Inactive
This gives these tests access to the same test helper APIs that the
other ones use. And, it means that we can access test helpers inside the
body of a `describe` block, which I’ll need in an upcoming commit in
order to call the shared delta tests added in 09354d4.
A summary of the discussion on #1492:

- the modular API should be the only way to pass optional functionality
  to the SDK

- this means we need to replace the existing ClientOptions.plugins
  mechanism, which is currently used to pass a Vcdiff decoder

- since the modular variant of the SDK only exists for web at the
  moment, we will bundle Vcdiff decoding into all other platforms (in
  which bundle size is not much of a concern)

- on web, if you want deltas, you have to use the modular variant of the
  SDK

So, we remove the ClientOptions.plugins mechanism and introduce a
tree-shakable Vcdiff module, which bundles the vcdiff-decoder library
(meaning that users no longer need to directly import this library).

Note that this means that, currently, it is no longer possible to use
deltas inside a Web Worker. We’ll address this in #1514.

The README example of configuring a channel to use deltas is copied from
the README of the vcdiff-decoder library.

(Once ably-js v2 is released, we should update the instructions in the
vcdiff-decoder library’s README to make it clear they only apply to v1.
I’ve raised #1513 for this.)

Resolves #1492.
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